| Message ID | 20230812091708.34665-3-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Document internal MDIO bus of DSA switch and support it on MT7530 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 9 this patch: 9 |
| netdev/cc_maintainers | success | CCed 18 of 18 maintainers |
| netdev/build_clang | success | Errors and warnings before: 9 this patch: 9 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| 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: 9 this patch: 9 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 102 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
I changed this to below. I will wait for reviews before submitting v2.
The realtek.yaml schema extends the mdio.yaml schema. The mdio.yaml schema
is also being referred to through dsa.yaml#/$defs/ethernet-ports now which
means we cannot disallow additional properties by 'unevaluatedProperties:
false' on the dsa.yaml schema.
On the realtek.yaml schema, refer to dsa.yaml#/properties/mdio instead to
point the human readers to the description on the dsa.yaml schema.
diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index ec74a660beda..03ccedbc49dc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -31,6 +31,24 @@ properties:
(single device hanging off a CPU port) must not specify this property
$ref: /schemas/types.yaml#/definitions/uint32-array
+ mdio:
+ description: The internal MDIO bus of the switch
+ $ref: /schemas/net/mdio.yaml#
+
+if:
+ required: [ mdio ]
+then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ if:
+ not:
+ required: [ ethernet ]
+ then:
+ required:
+ - phy-handle
+
additionalProperties: true
$defs:
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea..f4b4fe0509a0 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -96,7 +96,7 @@ properties:
- '#interrupt-cells'
mdio:
- $ref: /schemas/net/mdio.yaml#
+ $ref: dsa.yaml#/properties/mdio
unevaluatedProperties: false
properties:
Arınç
I've realised there are more schemas that extend the mdio.yaml schema. This
is the final state of this patch.
dt-bindings: net: dsa: document internal MDIO bus
Add the schema to document the internal MDIO bus. Require the phy-handle
property on the non-CPU ports if the mdio property is being used.
Define the mdio property on all of the schemas that refer to
dsa.yaml#/$defs/ethernet-ports. Refer to dsa.yaml#/properties/mdio to point
the human readers to the description on the dsa.yaml schema.
Some of these schemas extend the mdio.yaml schema. The mdio.yaml schema is
also being referred to through dsa.yaml#/$defs/ethernet-ports now which
means we cannot disallow additional properties by 'unevaluatedProperties:
false' on the dsa.yaml schema.
---
.../bindings/net/dsa/arrow,xrs700x.yaml | 4 ++++
.../devicetree/bindings/net/dsa/brcm,b53.yaml | 4 ++++
.../devicetree/bindings/net/dsa/brcm,sf2.yaml | 4 ++++
.../devicetree/bindings/net/dsa/dsa.yaml | 18 ++++++++++++++++++
.../bindings/net/dsa/hirschmann,hellcreek.yaml | 4 ++++
.../bindings/net/dsa/mediatek,mt7530.yaml | 4 ++++
.../bindings/net/dsa/microchip,ksz.yaml | 4 ++++
.../bindings/net/dsa/microchip,lan937x.yaml | 2 +-
.../bindings/net/dsa/mscc,ocelot.yaml | 4 ++++
.../bindings/net/dsa/nxp,sja1105.yaml | 4 ++++
.../devicetree/bindings/net/dsa/qca8k.yaml | 2 +-
.../devicetree/bindings/net/dsa/realtek.yaml | 2 +-
.../bindings/net/dsa/renesas,rzn1-a5psw.yaml | 2 +-
13 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
index 9565a740214629..f0229352e05694 100644
--- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
@@ -29,6 +29,10 @@ properties:
reg:
maxItems: 1
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
index 4c78c546343f5e..e14562b33bfb97 100644
--- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
@@ -65,6 +65,10 @@ properties:
- brcm,bcm63268-switch
- const: brcm,bcm63xx-switch
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
index c745407f2f6853..1bf4317e038687 100644
--- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
@@ -90,6 +90,10 @@ properties:
tags enabled (per-packet metadata)
type: boolean
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
required:
- reg
- interrupts
diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index ec74a660bedaed..03ccedbc49dcc3 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -31,6 +31,24 @@ properties:
(single device hanging off a CPU port) must not specify this property
$ref: /schemas/types.yaml#/definitions/uint32-array
+ mdio:
+ description: The internal MDIO bus of the switch
+ $ref: /schemas/net/mdio.yaml#
+
+if:
+ required: [ mdio ]
+then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ if:
+ not:
+ required: [ ethernet ]
+ then:
+ required:
+ - phy-handle
+
additionalProperties: true
$defs:
diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
index 4021b054f68446..32f17345825d4a 100644
--- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
@@ -67,6 +67,10 @@ properties:
additionalProperties: false
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index e532c6b795f4fc..293d1affe75451 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -151,6 +151,10 @@ properties:
ethsys.
maxItems: 1
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
patternProperties:
"^(ethernet-)?ports$":
type: object
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index e51be1ac036237..01d11c642ecfd4 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -49,6 +49,10 @@ properties:
Set if the output SYNCLKO clock should be disabled. Do not mix with
microchip,synclko-125.
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 49af4b0d591695..15f24a1716cd44 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -32,7 +32,7 @@ properties:
maxItems: 1
mdio:
- $ref: /schemas/net/mdio.yaml#
+ $ref: dsa.yaml#/properties/mdio
unevaluatedProperties: false
patternProperties:
diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
index fe02d05196e4a6..d781b8c2324836 100644
--- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
@@ -73,6 +73,10 @@ properties:
little-endian: true
big-endian: true
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 4d5f5cc6d031e2..82dda8fae8b16e 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -72,6 +72,10 @@ properties:
- compatible
- reg
+ mdio:
+ $ref: dsa.yaml#/properties/mdio
+ unevaluatedProperties: false
+
patternProperties:
"^(ethernet-)?ports$":
patternProperties:
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index df64eebebe1856..001b72bcd0746b 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -60,7 +60,7 @@ properties:
B68 on the QCA832x and B49 on the QCA833x.
mdio:
- $ref: /schemas/net/mdio.yaml#
+ $ref: dsa.yaml#/properties/mdio
unevaluatedProperties: false
description: Qca8k switch have an internal mdio to access switch port.
If this is not present, the legacy mapping is used and the
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea39..f4b4fe0509a022 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -96,7 +96,7 @@ properties:
- '#interrupt-cells'
mdio:
- $ref: /schemas/net/mdio.yaml#
+ $ref: dsa.yaml#/properties/mdio
unevaluatedProperties: false
properties:
diff --git a/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml b/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
index 833d2f68daa144..c58c4ec8613ac1 100644
--- a/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
@@ -46,7 +46,7 @@ properties:
maxItems: 1
mdio:
- $ref: /schemas/net/mdio.yaml#
+ $ref: dsa.yaml#/properties/mdio
unevaluatedProperties: false
clocks:
The nxp,sja1105.yaml schema also needed some changes.
dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings
SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus.
Therefore, disallow the mdios property for SJA1105, and the mdio property
for SJA1110.
Require the phy-handle property on the non-CPU ports if the mdios property
is being used.
Refer to dsa.yaml#/properties/mdio to point the human readers to the
description on the dsa.yaml schema.
---
.../bindings/net/dsa/nxp,sja1105.yaml | 20 ++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 82dda8fae8b16e..7d92350f1065b2 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -52,7 +52,7 @@ properties:
patternProperties:
"^mdio@[0-1]$":
- $ref: /schemas/net/mdio.yaml#
+ $ref: dsa.yaml#/properties/mdio
unevaluatedProperties: false
properties:
@@ -128,14 +128,32 @@ allOf:
then:
properties:
spi-cpol: false
+ mdios: false
+
required:
- spi-cpha
else:
properties:
spi-cpha: false
+ mdio: false
+
required:
- spi-cpol
+ - if:
+ required: [ mdios ]
+ then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ if:
+ not:
+ required: [ ethernet ]
+ then:
+ required:
+ - phy-handle
+
unevaluatedProperties: false
examples:
Arınç
On 12.08.2023 19:28, Arınç ÜNAL wrote:
> I changed this to below. I will wait for reviews before submitting v2.
>
> The realtek.yaml schema extends the mdio.yaml schema. The mdio.yaml schema
> is also being referred to through dsa.yaml#/$defs/ethernet-ports now which
> means we cannot disallow additional properties by 'unevaluatedProperties:
> false' on the dsa.yaml schema.
>
> On the realtek.yaml schema, refer to dsa.yaml#/properties/mdio instead to
> point the human readers to the description on the dsa.yaml schema.
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> index ec74a660beda..03ccedbc49dc 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
> @@ -31,6 +31,24 @@ properties:
> (single device hanging off a CPU port) must not specify this property
> $ref: /schemas/types.yaml#/definitions/uint32-array
>
> + mdio:
> + description: The internal MDIO bus of the switch
> + $ref: /schemas/net/mdio.yaml#
> +
> +if:
> + required: [ mdio ]
> +then:
> + patternProperties:
> + "^(ethernet-)?ports$":
> + patternProperties:
> + "^(ethernet-)?port@[0-9]+$":
> + if:
> + not:
> + required: [ ethernet ]
> + then:
> + required:
> + - phy-handle
> +
> additionalProperties: true
>
> $defs:
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cfd69c2604ea..f4b4fe0509a0 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -96,7 +96,7 @@ properties:
> - '#interrupt-cells'
>
> mdio:
> - $ref: /schemas/net/mdio.yaml#
> + $ref: dsa.yaml#/properties/mdio
> unevaluatedProperties: false
>
> properties:
>
> Arınç
On Sat, Aug 12, 2023 at 12:17:06PM +0300, Arınç ÜNAL wrote: > Add the schema to document the internal MDIO bus. Adjust realtek.yaml > accordingly. > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- > .../devicetree/bindings/net/dsa/dsa.yaml | 18 +++++ > .../devicetree/bindings/net/dsa/realtek.yaml | 66 +++++++++---------- > 2 files changed, 50 insertions(+), 34 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml > index ec74a660beda..03ccedbc49dc 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml > @@ -31,6 +31,24 @@ properties: > (single device hanging off a CPU port) must not specify this property > $ref: /schemas/types.yaml#/definitions/uint32-array > > + mdio: > + description: The internal MDIO bus of the switch > + $ref: /schemas/net/mdio.yaml# > + > +if: > + required: [ mdio ] > +then: > + patternProperties: > + "^(ethernet-)?ports$": > + patternProperties: > + "^(ethernet-)?port@[0-9]+$": > + if: > + not: > + required: [ ethernet ] To match only on user ports, this must also exclude DSA ports ("required: [ link ]"). > + then: > + required: > + - phy-handle No. The only thing permitted by the slave_mii_bus is to do something meaningful when phylink bindings ("phy-handle", "fixed-link" or "managed") are absent. But the presence of slave_mii_bus does not imply that user ports have a required phy-handle. They might be SerDes ports or xMII ports. So they might use "managed" or "fixed-link". The only thing that you can enforce is that, if the slave_mii_bus has an OF presence, then user ports must have phylink bindings. > + > additionalProperties: true > > $defs: > diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml > index cfd69c2604ea..ea7db0890abc 100644 > --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml > @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# > > title: Realtek switches for unmanaged switches > > -allOf: > - - $ref: dsa.yaml#/$defs/ethernet-ports > - > maintainers: > - Linus Walleij <linus.walleij@linaro.org> > > @@ -95,37 +92,38 @@ properties: > - '#address-cells' > - '#interrupt-cells' > > - mdio: > - $ref: /schemas/net/mdio.yaml# > - unevaluatedProperties: false > - > - properties: > - compatible: > - const: realtek,smi-mdio > - > -if: > - required: > - - reg > - > -then: > - $ref: /schemas/spi/spi-peripheral-props.yaml# > - not: > - required: > - - mdc-gpios > - - mdio-gpios > - - mdio > - > - properties: > - mdc-gpios: false > - mdio-gpios: false > - mdio: false > - > -else: > - required: > - - mdc-gpios > - - mdio-gpios > - - mdio > - - reset-gpios > +allOf: > + - $ref: dsa.yaml#/$defs/ethernet-ports > + - if: > + required: [ mdio ] > + then: > + properties: > + mdio: > + properties: > + compatible: > + const: realtek,smi-mdio > + > + - if: > + required: > + - reg > + then: > + $ref: /schemas/spi/spi-peripheral-props.yaml# > + not: > + required: > + - mdc-gpios > + - mdio-gpios > + - mdio > + > + properties: > + mdc-gpios: false > + mdio-gpios: false > + mdio: false > + else: > + required: > + - mdc-gpios > + - mdio-gpios > + - mdio > + - reset-gpios > > required: > - compatible > -- > 2.39.2 >
On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote: > diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > index 4d5f5cc6d031e2..82dda8fae8b16e 100644 > --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > @@ -72,6 +72,10 @@ properties: > - compatible > - reg > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false sja1105 does not support an "mdio" child property. I haven't checked the others. Don't add properties that aren't supported. > + > patternProperties: > "^(ethernet-)?ports$": > patternProperties: > > The nxp,sja1105.yaml schema also needed some changes. > > dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings > > SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus. > Therefore, disallow the mdios property for SJA1105, and the mdio property > for SJA1110. > > Require the phy-handle property on the non-CPU ports if the mdios property > is being used. > > Refer to dsa.yaml#/properties/mdio to point the human readers to the > description on the dsa.yaml schema. > > --- > .../bindings/net/dsa/nxp,sja1105.yaml | 20 ++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > index 82dda8fae8b16e..7d92350f1065b2 100644 > --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > @@ -52,7 +52,7 @@ properties: > patternProperties: > "^mdio@[0-1]$": > - $ref: /schemas/net/mdio.yaml# > + $ref: dsa.yaml#/properties/mdio > unevaluatedProperties: false > properties: > @@ -128,14 +128,32 @@ allOf: > then: > properties: > spi-cpol: false > + mdios: false > + > required: > - spi-cpha > else: > properties: > spi-cpha: false > + mdio: false > + > required: > - spi-cpol > + - if: > + required: [ mdios ] > + then: > + patternProperties: > + "^(ethernet-)?ports$": > + patternProperties: > + "^(ethernet-)?port@[0-9]+$": > + if: > + not: > + required: [ ethernet ] > + then: > + required: > + - phy-handle For sja1105, phylink-compatible bindings (phy-handle, fixed-link or managed) are required for all ports (user, dsa or cpu). Also, sja1105 does not populate the slave_mii_bus, so it never uses the fallback where ports implicitly connect to an internal PHY if no phylink bindings are present. > + > unevaluatedProperties: false > examples: > > Arınç
On 13.08.2023 14:15, Vladimir Oltean wrote: > On Sat, Aug 12, 2023 at 12:17:06PM +0300, Arınç ÜNAL wrote: >> Add the schema to document the internal MDIO bus. Adjust realtek.yaml >> accordingly. >> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- >> .../devicetree/bindings/net/dsa/dsa.yaml | 18 +++++ >> .../devicetree/bindings/net/dsa/realtek.yaml | 66 +++++++++---------- >> 2 files changed, 50 insertions(+), 34 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> index ec74a660beda..03ccedbc49dc 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> @@ -31,6 +31,24 @@ properties: >> (single device hanging off a CPU port) must not specify this property >> $ref: /schemas/types.yaml#/definitions/uint32-array >> >> + mdio: >> + description: The internal MDIO bus of the switch >> + $ref: /schemas/net/mdio.yaml# >> + >> +if: >> + required: [ mdio ] >> +then: >> + patternProperties: >> + "^(ethernet-)?ports$": >> + patternProperties: >> + "^(ethernet-)?port@[0-9]+$": >> + if: >> + not: >> + required: [ ethernet ] > > To match only on user ports, this must also exclude DSA ports ("required: [ link ]"). > >> + then: >> + required: >> + - phy-handle > > No. The only thing permitted by the slave_mii_bus is to do something meaningful > when phylink bindings ("phy-handle", "fixed-link" or "managed") are absent. But > the presence of slave_mii_bus does not imply that user ports have a required > phy-handle. They might be SerDes ports or xMII ports. So they might use "managed" > or "fixed-link". The only thing that you can enforce is that, if the slave_mii_bus > has an OF presence, then user ports must have phylink bindings. Got it. This should be the correct schema then. I don't check for ethernet or link as when the mdio property is used, these bindings apply to all ports. DSA and CPU ports are then further restricted with the dsa-port.yaml schema. if: required: [ mdio ] then: patternProperties: "^(ethernet-)?ports$": patternProperties: "^(ethernet-)?port@[0-9]+$": oneOf: - required: - fixed-link - required: - phy-handle - required: - managed Arınç
On 13.08.2023 14:53, Vladimir Oltean wrote: > On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote: >> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> index 4d5f5cc6d031e2..82dda8fae8b16e 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> @@ -72,6 +72,10 @@ properties: >> - compatible >> - reg >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false > > sja1105 does not support an "mdio" child property. I haven't checked the > others. Don't add properties that aren't supported. Adding the mdio property to the dsa.yaml schema will allow it on all of the schemas that refer to dsa.yaml such as this one. This addition here is only to disallow additional properties under the mdio property for this specific schema. That said, my understanding is that the internal MDIO bus exists on all of the switches controlled by DSA. Whether each individual DSA subdriver supports registering it does not matter in terms of documenting the internal MDIO bus for all DSA switches. SJA1110 uses the mdios property instead because it's got two internal mdio buses, which is why I invalidate the mdio property for it. If SJA1105 has also got two internal mdio buses, let me know. > >> + >> patternProperties: >> "^(ethernet-)?ports$": >> patternProperties: >> >> The nxp,sja1105.yaml schema also needed some changes. >> >> dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings >> >> SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus. >> Therefore, disallow the mdios property for SJA1105, and the mdio property >> for SJA1110. >> >> Require the phy-handle property on the non-CPU ports if the mdios property >> is being used. >> >> Refer to dsa.yaml#/properties/mdio to point the human readers to the >> description on the dsa.yaml schema. >> >> --- >> .../bindings/net/dsa/nxp,sja1105.yaml | 20 ++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> index 82dda8fae8b16e..7d92350f1065b2 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> @@ -52,7 +52,7 @@ properties: >> patternProperties: >> "^mdio@[0-1]$": >> - $ref: /schemas/net/mdio.yaml# >> + $ref: dsa.yaml#/properties/mdio >> unevaluatedProperties: false >> properties: >> @@ -128,14 +128,32 @@ allOf: >> then: >> properties: >> spi-cpol: false >> + mdios: false >> + >> required: >> - spi-cpha >> else: >> properties: >> spi-cpha: false >> + mdio: false >> + >> required: >> - spi-cpol >> + - if: >> + required: [ mdios ] >> + then: >> + patternProperties: >> + "^(ethernet-)?ports$": >> + patternProperties: >> + "^(ethernet-)?port@[0-9]+$": >> + if: >> + not: >> + required: [ ethernet ] >> + then: >> + required: >> + - phy-handle > > For sja1105, phylink-compatible bindings (phy-handle, fixed-link or managed) > are required for all ports (user, dsa or cpu). > > Also, sja1105 does not populate the slave_mii_bus, so it never uses the > fallback where ports implicitly connect to an internal PHY if no phylink > bindings are present. I'll handle these accordingly with your answer to my question above. Arınç
On 13.08.2023 15:59, Arınç ÜNAL wrote: > On 13.08.2023 14:53, Vladimir Oltean wrote: >> On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote: >>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >>> index 4d5f5cc6d031e2..82dda8fae8b16e 100644 >>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >>> @@ -72,6 +72,10 @@ properties: >>> - compatible >>> - reg >>> + mdio: >>> + $ref: dsa.yaml#/properties/mdio >>> + unevaluatedProperties: false >> >> sja1105 does not support an "mdio" child property. I haven't checked the >> others. Don't add properties that aren't supported. > > Adding the mdio property to the dsa.yaml schema will allow it on all of the > schemas that refer to dsa.yaml such as this one. This addition here is only > to disallow additional properties under the mdio property for this specific > schema. > > That said, my understanding is that the internal MDIO bus exists on all of > the switches controlled by DSA. Whether each individual DSA subdriver > supports registering it does not matter in terms of documenting the > internal MDIO bus for all DSA switches. On top of this, I'd argue to document the internal MDIO bus on the ethernet-switch.yaml schema instead. Arınç > > SJA1110 uses the mdios property instead because it's got two internal mdio > buses, which is why I invalidate the mdio property for it. If SJA1105 has > also got two internal mdio buses, let me know. > >> >>> + >>> patternProperties: >>> "^(ethernet-)?ports$": >>> patternProperties: >>> >>> The nxp,sja1105.yaml schema also needed some changes. >>> >>> dt-bindings: net: dsa: nxp,sja1105: improve internal MDIO bus bindings >>> >>> SJA1110 Ethernet Switch uses the mdios property for its internal MDIO bus. >>> Therefore, disallow the mdios property for SJA1105, and the mdio property >>> for SJA1110. >>> >>> Require the phy-handle property on the non-CPU ports if the mdios property >>> is being used. >>> >>> Refer to dsa.yaml#/properties/mdio to point the human readers to the >>> description on the dsa.yaml schema. >>> >>> --- >>> .../bindings/net/dsa/nxp,sja1105.yaml | 20 ++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >>> index 82dda8fae8b16e..7d92350f1065b2 100644 >>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >>> @@ -52,7 +52,7 @@ properties: >>> patternProperties: >>> "^mdio@[0-1]$": >>> - $ref: /schemas/net/mdio.yaml# >>> + $ref: dsa.yaml#/properties/mdio >>> unevaluatedProperties: false >>> properties: >>> @@ -128,14 +128,32 @@ allOf: >>> then: >>> properties: >>> spi-cpol: false >>> + mdios: false >>> + >>> required: >>> - spi-cpha >>> else: >>> properties: >>> spi-cpha: false >>> + mdio: false >>> + >>> required: >>> - spi-cpol >>> + - if: >>> + required: [ mdios ] >>> + then: >>> + patternProperties: >>> + "^(ethernet-)?ports$": >>> + patternProperties: >>> + "^(ethernet-)?port@[0-9]+$": >>> + if: >>> + not: >>> + required: [ ethernet ] >>> + then: >>> + required: >>> + - phy-handle >> >> For sja1105, phylink-compatible bindings (phy-handle, fixed-link or managed) >> are required for all ports (user, dsa or cpu). >> >> Also, sja1105 does not populate the slave_mii_bus, so it never uses the >> fallback where ports implicitly connect to an internal PHY if no phylink >> bindings are present. > > I'll handle these accordingly with your answer to my question above. > > Arınç
On Sun, Aug 13, 2023 at 03:59:11PM +0300, Arınç ÜNAL wrote: > > sja1105 does not support an "mdio" child property. I haven't checked the > > others. Don't add properties that aren't supported. > > Adding the mdio property to the dsa.yaml schema will allow it on all of the > schemas that refer to dsa.yaml such as this one. This addition here is only > to disallow additional properties under the mdio property for this specific > schema. > > That said, my understanding is that the internal MDIO bus exists on all of > the switches controlled by DSA. How come? > Whether each individual DSA subdriver supports registering it does not > matter in terms of documenting the internal MDIO bus for all DSA > switches. > > SJA1110 uses the mdios property instead because it's got two internal mdio > buses, which is why I invalidate the mdio property for it. If SJA1105 has > also got two internal mdio buses, let me know. SJA1105 has zero internal MDIO buses and zero internal PHYs.
On Sun, Aug 13, 2023 at 05:58:57PM +0300, Arınç ÜNAL wrote: > On top of this, I'd argue to document the internal MDIO bus on the > ethernet-switch.yaml schema instead. Why?
On 13.08.2023 22:02, Vladimir Oltean wrote: > On Sun, Aug 13, 2023 at 05:58:57PM +0300, Arınç ÜNAL wrote: >> On top of this, I'd argue to document the internal MDIO bus on the >> ethernet-switch.yaml schema instead. > > Why? Because a generic switch can have an internal MDIO bus, it's not specific to a DSA controlled switch. Arınç
On 13.08.2023 22:01, Vladimir Oltean wrote: > On Sun, Aug 13, 2023 at 03:59:11PM +0300, Arınç ÜNAL wrote: >>> sja1105 does not support an "mdio" child property. I haven't checked the >>> others. Don't add properties that aren't supported. >> >> Adding the mdio property to the dsa.yaml schema will allow it on all of the >> schemas that refer to dsa.yaml such as this one. This addition here is only >> to disallow additional properties under the mdio property for this specific >> schema. >> >> That said, my understanding is that the internal MDIO bus exists on all of >> the switches controlled by DSA. > > How come? > >> Whether each individual DSA subdriver supports registering it does not >> matter in terms of documenting the internal MDIO bus for all DSA >> switches. >> >> SJA1110 uses the mdios property instead because it's got two internal mdio >> buses, which is why I invalidate the mdio property for it. If SJA1105 has >> also got two internal mdio buses, let me know. > > SJA1105 has zero internal MDIO buses and zero internal PHYs. Ah okay. I didn't consider the switch architecture where the data interface of the PHY is connected to the switch, and the PHY management interface is connected to the mdio bus that the switch is connected to. The schemas of the switches which already utilise the mdio property: - /schemas/net/dsa/microchip,lan937x.yaml - /schemas/net/dsa/qca8k.yaml - /schemas/net/dsa/realtek.yaml - /schemas/net/dsa/renesas,rzn1-a5psw.yaml The schemas of the switches which don't have an internal MDIO bus, meaning the mdio property must be invalid: - /schemas/net/mscc,vsc7514-switch.yaml - /schemas/net/dsa/nxp,sja1105.yaml The schemas of the switches which I don't know if the switch has got an internal MDIO bus: - /schemas/net/dsa/arrow,xrs700x.yaml - I believe, because there's phy-handle defined on every port without the mdio node on the example, the PHYs are not connected to the internal MDIO bus. Therefore, we can invalidate the mdio property for this schema. - /schemas/net/dsa/brcm,b53.yaml - Seems ok to keep it valid. - /schemas/net/dsa/brcm,sf2.yaml - Seems ok to keep it valid. - /schemas/net/dsa/hirschmann,hellcreek.yaml - Same as /schemas/net/dsa/arrow,xrs700x.yaml. - /schemas/net/dsa/microchip,ksz.yaml - Seems ok to keep it valid. - /schemas/net/dsa/mscc,ocelot.yaml - Same as /schemas/net/dsa/arrow,xrs700x.yaml. Not json-schema documentation, don't care about: - ar9331.txt - lan9303.txt - lantiq-gswip.txt - marvell.txt - vitesse,vsc73xx.txt Arınç
On Mon, Aug 14, 2023 at 01:06:19PM +0300, Arınç ÜNAL wrote: > On 13.08.2023 22:02, Vladimir Oltean wrote: > > On Sun, Aug 13, 2023 at 05:58:57PM +0300, Arınç ÜNAL wrote: > > > On top of this, I'd argue to document the internal MDIO bus on the > > > ethernet-switch.yaml schema instead. > > > > Why? > > Because a generic switch can have an internal MDIO bus, it's not specific > to a DSA controlled switch. > > Arınç I'm not sure it's that simple. Check out arch/mips/boot/dts/mscc/ocelot.dtsi. Its switch IP ("mscc,vsc7514-switch") is on the same hierarchical level with the "mscc,ocelot-miim" nodes (so, not a child), because the MDIO controller isn't part of the address space of the switching IP. Actually that could equally be considered true for DSA. Placing the "mdio" node as a child of the switch is one of the possible options, but it has its limitations and is certainly not the only one.
> Ah okay. I didn't consider the switch architecture where the data interface > of the PHY is connected to the switch, and the PHY management interface is > connected to the mdio bus that the switch is connected to. The generic Linux architecture for PHYs and binding them to a MAC via a phandle allows the PHY to be on any MDIO bus anywhere. DSA has some additional shortcuts to support 1:1 mapping if the switch has its own MDIO bus, without describing it in DT, but this is just in addition to the generic code. > Not json-schema documentation, don't care about: > - ar9331.txt > - lan9303.txt > - lantiq-gswip.txt > - marvell.txt The marvell switch can have up to 2 MDIO busses. If i remember correctly, there is also one switch which has one MDIO bus per port. Andrew
Arınç, On Mon, Aug 14, 2023 at 01:06:29PM +0300, Arınç ÜNAL wrote: > On 13.08.2023 22:01, Vladimir Oltean wrote: > > SJA1105 has zero internal MDIO buses and zero internal PHYs. > > Ah okay. I didn't consider the switch architecture where the data interface > of the PHY is connected to the switch, and the PHY management interface is > connected to the mdio bus that the switch is connected to. > > The schemas of the switches which already utilise the mdio property: > - /schemas/net/dsa/microchip,lan937x.yaml > - /schemas/net/dsa/qca8k.yaml > - /schemas/net/dsa/realtek.yaml > - /schemas/net/dsa/renesas,rzn1-a5psw.yaml > > The schemas of the switches which don't have an internal MDIO bus, meaning > the mdio property must be invalid: > - /schemas/net/mscc,vsc7514-switch.yaml > - /schemas/net/dsa/nxp,sja1105.yaml > > The schemas of the switches which I don't know if the switch has got an > internal MDIO bus: > - /schemas/net/dsa/arrow,xrs700x.yaml > - I believe, because there's phy-handle defined on every port without the > mdio node on the example, the PHYs are not connected to the internal > MDIO bus. Therefore, we can invalidate the mdio property for this > schema. > - /schemas/net/dsa/brcm,b53.yaml > - Seems ok to keep it valid. > - /schemas/net/dsa/brcm,sf2.yaml > - Seems ok to keep it valid. > - /schemas/net/dsa/hirschmann,hellcreek.yaml > - Same as /schemas/net/dsa/arrow,xrs700x.yaml. > - /schemas/net/dsa/microchip,ksz.yaml > - Seems ok to keep it valid. > - /schemas/net/dsa/mscc,ocelot.yaml > - Same as /schemas/net/dsa/arrow,xrs700x.yaml. > > Not json-schema documentation, don't care about: > - ar9331.txt > - lan9303.txt > - lantiq-gswip.txt > - marvell.txt > - vitesse,vsc73xx.txt > > Arınç We have to keep in sight why we're here, and stick to that. You had issues with a device tree that didn't work, but it passed validation, and you're trying to enforce extra rules through the json-schema so that next time, it fails. Verbally, that rule would be: "if the switch has a ds->slave_mii_bus which does not have an OF presence, then phylink compatible bindings may be omitted, and that has a special and valid meaning (the port is connected to an internal PHY on that ds->slave_mii_bus). If ds->slave_mii_bus has an OF presence, or if ds->slave_mii_bus is NULL, then phylink-compatible bindings (phy-handle or fixed-link or managed) are required on all user ports". So it becomes a question of tracking ds->slave_mii_bus for all drivers. In essence, it's fundamentally about the ds->slave_mii_bus, not about the "mdio" child node. See more below. There are 2 code paths that lead to its creation: 1. DSA registers the bus in dsa_slave_mii_bus_init(), based on the presence of ds->ops->phy_read() and ds->ops->phy_write(). Traditionally, a slave_mii_bus created this way was always non-OF-based, but Luiz, in commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), thought it would be a good idea for them to be optionally OF-based (and thus, useless at their primary purpose of being able to have internal PHYs without a phy-handle). But, it was thought that the framework registering an MDIO bus automatically would be a plus. So, ds->slave_mii_bus created in this way may or may not have an OF presence, with no way to know except to look at device trees (and to presume that they do). The drivers that populate ds->ops->phy_read() and ds->ops->phy_write() are: | +--- dsa_loop_driver: not OF-based | +--- ksz_switch_ops: OF-based or non-OF-based | +--- mv88e6060_switch_ops: OF-based or non-OF-based | +--- lan9303_switch_ops: OF-based or non-OF-based | +--- rtl8365mb_switch_ops_mdio: OF-based or non-OF-based | +--- b53_switch_ops: OF-based or non-OF-based | +--- vsc73xx_ds_ops: OF-based or non-OF-based 2. The switch driver registers the bus, and populates ds->slave_mii_bus with a pointer to it. | +--- Bus is not OF-based (it was registered with mdiobus_register()). | This is the normal case: | * mv88e6xxx_default_mdio_bus() in some cases | * qca8k_mdio_register() in the "qca8k-legacy slave mii" case | * bcm_sf2_mdio_register() | * mt7530_setup_mdio() | +--- Bus is OF-based (it was registered with of_mdiobus_register()). I've no idea why you'd do this, because you have neither the benefit of using a non-OF-based phy_connect(), nor the benefit of having DSA register the bus for you: * mv88e6xxx_default_mdio_bus() when of_get_child_by_name(np, "mdio") is non-NULL * qca8k_mdio_register() when of_get_child_by_name(priv->dev->of_node, "mdio") is non-NULL * ksz_mdio_register() - it always wants an "mdio" child node * gswip_mdio() - it always wants a child node compatible with "lantiq,xrx200-mdio" * realtek_smi_setup_mdio() - it always wants a child node compatible with "realtek,smi-mdio" For switches in the first category, the presence of the "mdio" child node is what makes the ds->slave_mii_bus be OF-based or not, since it is all the same binding, imposed by Luiz in dsa_switch_setup(). For switches in the second category, it all depends on the way in which the driver finds the node for of_mdiobus_register(). Having identified all switches which make some sort of use of ds->slave_mii_bus, the rule would sound like this: 1. If the schema is that of (need to replace this with compatible strings, I'm too lazy for that): - ksz_switch_ops - mv88e6060_switch_ops - lan9303_switch_ops - rtl8365mb_switch_ops_mdio - b53_switch_ops - vsc73xx_ds_ops - mv88e6xxx - qca8k and we have an "mdio" child, then phylink bindings are mandatory on user ports. 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", then phylink bindings are mandatory on user ports (I haven't checked, but it might be that the "lantiq,xrx200-mdio" child is mandatory, and in that case, this goes to category 4 below). 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of "realtek,smi-mdio", then phylink bindings are mandatory on user ports (same comment about the child MDIO note maybe being mandatory). 4. If the switch didn't appear in the above set of rules, then phylink bindings are unconditionally mandatory on user ports. We don't care at all what the drivers that don't use ds->slave_mii_bus do with the "mdio" child node. It doesn't change the fact that their user ports can't have missing phylink bindings.
On Sun, Aug 13, 2023 at 03:59:11PM +0300, Arınç ÜNAL wrote: > Adding the mdio property to the dsa.yaml schema will allow it on all of the > schemas that refer to dsa.yaml such as this one. This addition here is only > to disallow additional properties under the mdio property for this specific > schema. So, how about not adding it to dsa.yaml, but to individual switch schemas, along with their specific handling of phylink bindings on user ports?
On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote: > I've realised there are more schemas that extend the mdio.yaml schema. This > is the final state of this patch. > > dt-bindings: net: dsa: document internal MDIO bus > > Add the schema to document the internal MDIO bus. Require the phy-handle > property on the non-CPU ports if the mdio property is being used. > > Define the mdio property on all of the schemas that refer to > dsa.yaml#/$defs/ethernet-ports. Refer to dsa.yaml#/properties/mdio to point > the human readers to the description on the dsa.yaml schema. > > Some of these schemas extend the mdio.yaml schema. The mdio.yaml schema is > also being referred to through dsa.yaml#/$defs/ethernet-ports now which > means we cannot disallow additional properties by 'unevaluatedProperties: > false' on the dsa.yaml schema. > > --- > .../bindings/net/dsa/arrow,xrs700x.yaml | 4 ++++ > .../devicetree/bindings/net/dsa/brcm,b53.yaml | 4 ++++ > .../devicetree/bindings/net/dsa/brcm,sf2.yaml | 4 ++++ > .../devicetree/bindings/net/dsa/dsa.yaml | 18 ++++++++++++++++++ > .../bindings/net/dsa/hirschmann,hellcreek.yaml | 4 ++++ > .../bindings/net/dsa/mediatek,mt7530.yaml | 4 ++++ > .../bindings/net/dsa/microchip,ksz.yaml | 4 ++++ > .../bindings/net/dsa/microchip,lan937x.yaml | 2 +- > .../bindings/net/dsa/mscc,ocelot.yaml | 4 ++++ > .../bindings/net/dsa/nxp,sja1105.yaml | 4 ++++ > .../devicetree/bindings/net/dsa/qca8k.yaml | 2 +- > .../devicetree/bindings/net/dsa/realtek.yaml | 2 +- > .../bindings/net/dsa/renesas,rzn1-a5psw.yaml | 2 +- > 13 files changed, 54 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml > index 9565a740214629..f0229352e05694 100644 > --- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml > @@ -29,6 +29,10 @@ properties: > reg: > maxItems: 1 > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > required: > - compatible > - reg > diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml > index 4c78c546343f5e..e14562b33bfb97 100644 > --- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml > @@ -65,6 +65,10 @@ properties: > - brcm,bcm63268-switch > - const: brcm,bcm63xx-switch > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > required: > - compatible > - reg > diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml > index c745407f2f6853..1bf4317e038687 100644 > --- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml > @@ -90,6 +90,10 @@ properties: > tags enabled (per-packet metadata) > type: boolean > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > required: > - reg > - interrupts > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml > index ec74a660bedaed..03ccedbc49dcc3 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml > @@ -31,6 +31,24 @@ properties: > (single device hanging off a CPU port) must not specify this property > $ref: /schemas/types.yaml#/definitions/uint32-array > + mdio: > + description: The internal MDIO bus of the switch > + $ref: /schemas/net/mdio.yaml# > + > +if: > + required: [ mdio ] > +then: > + patternProperties: > + "^(ethernet-)?ports$": > + patternProperties: > + "^(ethernet-)?port@[0-9]+$": > + if: > + not: > + required: [ ethernet ] > + then: > + required: > + - phy-handle > + > additionalProperties: true > $defs: > diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml > index 4021b054f68446..32f17345825d4a 100644 > --- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml > @@ -67,6 +67,10 @@ properties: > additionalProperties: false > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > required: > - compatible > - reg > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > index e532c6b795f4fc..293d1affe75451 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > @@ -151,6 +151,10 @@ properties: > ethsys. > maxItems: 1 > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > patternProperties: > "^(ethernet-)?ports$": > type: object > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > index e51be1ac036237..01d11c642ecfd4 100644 > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > @@ -49,6 +49,10 @@ properties: > Set if the output SYNCLKO clock should be disabled. Do not mix with > microchip,synclko-125. > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > required: > - compatible > - reg > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml > index 49af4b0d591695..15f24a1716cd44 100644 > --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml > @@ -32,7 +32,7 @@ properties: > maxItems: 1 > mdio: > - $ref: /schemas/net/mdio.yaml# > + $ref: dsa.yaml#/properties/mdio > unevaluatedProperties: false > patternProperties: > diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > index fe02d05196e4a6..d781b8c2324836 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml > @@ -73,6 +73,10 @@ properties: > little-endian: true > big-endian: true > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > required: > - compatible > - reg > diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > index 4d5f5cc6d031e2..82dda8fae8b16e 100644 > --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > @@ -72,6 +72,10 @@ properties: > - compatible > - reg > + mdio: > + $ref: dsa.yaml#/properties/mdio > + unevaluatedProperties: false > + > patternProperties: > "^(ethernet-)?ports$": > patternProperties: > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > index df64eebebe1856..001b72bcd0746b 100644 > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > @@ -60,7 +60,7 @@ properties: > B68 on the QCA832x and B49 on the QCA833x. > mdio: > - $ref: /schemas/net/mdio.yaml# > + $ref: dsa.yaml#/properties/mdio > unevaluatedProperties: false Just from a schema standpoint, this is pointless indirection as dsa.yaml#/properties/mdio is just a reference to /schemas/net/mdio.yaml#. As it seems an MDIO bus is not universal for DSA, it seems you'll be dropping this change anyways. Rob
On 14.08.2023 17:36, Vladimir Oltean wrote: > Arınç, > > On Mon, Aug 14, 2023 at 01:06:29PM +0300, Arınç ÜNAL wrote: >> On 13.08.2023 22:01, Vladimir Oltean wrote: >>> SJA1105 has zero internal MDIO buses and zero internal PHYs. >> >> Ah okay. I didn't consider the switch architecture where the data interface >> of the PHY is connected to the switch, and the PHY management interface is >> connected to the mdio bus that the switch is connected to. >> >> The schemas of the switches which already utilise the mdio property: >> - /schemas/net/dsa/microchip,lan937x.yaml >> - /schemas/net/dsa/qca8k.yaml >> - /schemas/net/dsa/realtek.yaml >> - /schemas/net/dsa/renesas,rzn1-a5psw.yaml >> >> The schemas of the switches which don't have an internal MDIO bus, meaning >> the mdio property must be invalid: >> - /schemas/net/mscc,vsc7514-switch.yaml >> - /schemas/net/dsa/nxp,sja1105.yaml >> >> The schemas of the switches which I don't know if the switch has got an >> internal MDIO bus: >> - /schemas/net/dsa/arrow,xrs700x.yaml >> - I believe, because there's phy-handle defined on every port without the >> mdio node on the example, the PHYs are not connected to the internal >> MDIO bus. Therefore, we can invalidate the mdio property for this >> schema. >> - /schemas/net/dsa/brcm,b53.yaml >> - Seems ok to keep it valid. >> - /schemas/net/dsa/brcm,sf2.yaml >> - Seems ok to keep it valid. >> - /schemas/net/dsa/hirschmann,hellcreek.yaml >> - Same as /schemas/net/dsa/arrow,xrs700x.yaml. >> - /schemas/net/dsa/microchip,ksz.yaml >> - Seems ok to keep it valid. >> - /schemas/net/dsa/mscc,ocelot.yaml >> - Same as /schemas/net/dsa/arrow,xrs700x.yaml. >> >> Not json-schema documentation, don't care about: >> - ar9331.txt >> - lan9303.txt >> - lantiq-gswip.txt >> - marvell.txt >> - vitesse,vsc73xx.txt >> >> Arınç > > We have to keep in sight why we're here, and stick to that. > > You had issues with a device tree that didn't work, but it passed > validation, and you're trying to enforce extra rules through the > json-schema so that next time, it fails. Verbally, that rule would be: > "if the switch has a ds->slave_mii_bus which does not have an OF > presence, then phylink compatible bindings may be omitted, and that has > a special and valid meaning (the port is connected to an internal PHY on > that ds->slave_mii_bus). If ds->slave_mii_bus has an OF presence, or if > ds->slave_mii_bus is NULL, then phylink-compatible bindings (phy-handle > or fixed-link or managed) are required on all user ports". > > So it becomes a question of tracking ds->slave_mii_bus for all drivers. > In essence, it's fundamentally about the ds->slave_mii_bus, not about > the "mdio" child node. See more below. Before I continue commenting, I'd like to state my understanding so we can make sure we're on the same page. If a driver doesn't use ds->slave_mii_bus, the switch it controls must not have any internal MDIO buses. Otherwise the PHYs on these buses couldn't function, and an improper driver like this would not be on the official Linux source code. I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC with a switch component. Since the switch component is not designed to be a standalone IC, the MDIO bus of the SoC could just as well be used without the need to design an MDIO controller specific to the switch component, to manage the PHYs. So I see this switch as just another case of a switch without an internal MDIO bus. > > There are 2 code paths that lead to its creation: > > 1. DSA registers the bus in dsa_slave_mii_bus_init(), based on the > presence of ds->ops->phy_read() and ds->ops->phy_write(). Traditionally, > a slave_mii_bus created this way was always non-OF-based, but Luiz, > in commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), thought > it would be a good idea for them to be optionally OF-based (and thus, > useless at their primary purpose of being able to have internal PHYs > without a phy-handle). But, it was thought that the framework > registering an MDIO bus automatically would be a plus. So, ds->slave_mii_bus > created in this way may or may not have an OF presence, with no way > to know except to look at device trees (and to presume that they do). > > The drivers that populate ds->ops->phy_read() and ds->ops->phy_write() are: > | > +--- dsa_loop_driver: not OF-based > | > +--- ksz_switch_ops: OF-based or non-OF-based > | > +--- mv88e6060_switch_ops: OF-based or non-OF-based > | > +--- lan9303_switch_ops: OF-based or non-OF-based > | > +--- rtl8365mb_switch_ops_mdio: OF-based or non-OF-based > | > +--- b53_switch_ops: OF-based or non-OF-based > | > +--- vsc73xx_ds_ops: OF-based or non-OF-based > > 2. The switch driver registers the bus, and populates ds->slave_mii_bus with > a pointer to it. > | > +--- Bus is not OF-based (it was registered with mdiobus_register()). > | This is the normal case: > | * mv88e6xxx_default_mdio_bus() in some cases > | * qca8k_mdio_register() in the "qca8k-legacy slave mii" case > | * bcm_sf2_mdio_register() > | * mt7530_setup_mdio() > | > +--- Bus is OF-based (it was registered with of_mdiobus_register()). > I've no idea why you'd do this, because you have neither the > benefit of using a non-OF-based phy_connect(), nor the benefit > of having DSA register the bus for you: > * mv88e6xxx_default_mdio_bus() when of_get_child_by_name(np, "mdio") > is non-NULL > * qca8k_mdio_register() when of_get_child_by_name(priv->dev->of_node, "mdio") > is non-NULL > * ksz_mdio_register() - it always wants an "mdio" child node > * gswip_mdio() - it always wants a child node compatible with > "lantiq,xrx200-mdio" > * realtek_smi_setup_mdio() - it always wants a child node > compatible with "realtek,smi-mdio" > > For switches in the first category, the presence of the "mdio" child > node is what makes the ds->slave_mii_bus be OF-based or not, since it is > all the same binding, imposed by Luiz in dsa_switch_setup(). Makes sense. > > For switches in the second category, it all depends on the way in which > the driver finds the node for of_mdiobus_register(). Ok, so some drivers require the mdio child node. Some require it and the compatible property with a certain string. MDIO controlled Realtek switches do not need the compatible property under the mdio child node. There're no compatible strings to make a distinction between the SMI and MDIO controlled switches so the best we can do is keep it the way it currently is. Define realtek,smi-mdio as a compatible string but keep the compatible property optional. I did state this on my reply to patch 3 but still received reviewed-bys regardless. > > > Having identified all switches which make some sort of use of > ds->slave_mii_bus, the rule would sound like this: > > 1. If the schema is that of (need to replace this with compatible > strings, I'm too lazy for that): > > - ksz_switch_ops > - mv88e6060_switch_ops > - lan9303_switch_ops > - rtl8365mb_switch_ops_mdio > - b53_switch_ops > - vsc73xx_ds_ops > - mv88e6xxx > - qca8k > > and we have an "mdio" child, then phylink bindings are mandatory on user ports. > > 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", > then phylink bindings are mandatory on user ports (I haven't checked, > but it might be that the "lantiq,xrx200-mdio" child is mandatory, and > in that case, this goes to category 4 below). > > 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of > "realtek,smi-mdio", then phylink bindings are mandatory on user ports > (same comment about the child MDIO note maybe being mandatory). > > 4. If the switch didn't appear in the above set of rules, then phylink > bindings are unconditionally mandatory on user ports. > > We don't care at all what the drivers that don't use ds->slave_mii_bus > do with the "mdio" child node. It doesn't change the fact that their > user ports can't have missing phylink bindings. I partially agree. I say, for the switches without an internal MDIO bus, invalidate the mdio child node, and enforce the phylink bindings on the user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x, invalidate the mdio child node, and enforce the phylink bindings on the user ports if the mdios property is used. I'd like to add this before I conclude. The way I understand dt-bindings is that a binding does not have to translate to an action on the driver. Documenting bindings for the sole purpose of describing hardware is a valid case. For example, currently, the MT753X DSA subdriver won't, in any way, register the bus OF-based. Still, the mdio property for the switches which this driver controls can be documented because the internal mdio bus does exist on the hardware. So I'd like to keep the mdio property valid for the switches which their drivers can only register non-OF-based ds->slave_mii_bus. In conclusion, what to do: - Define "the mdio property" and "the enforcement of phylink bindings for user ports if mdio property is used" on ethernet-switch.yaml. - Invalidate the mdio property on the switches without an internal MDIO bus. - Define "the enforcement of phylink bindings for user ports" on the switches without an internal MDIO bus. - Require "the mdio property" for the switches which their driver requires it to function. - Require "the mdio property" and "the compatible string of the mdio property" for the switches which their driver requires them to function. There's no 1:1 switch to switch compatible string relation, as seen on Realtek switches so I'll have to figure that out as I go. I'm open to your comments to this mail but the gap between discussion and end result has widened a lot on this patch series so I'd like to first offload this conversation by preparing v2 with what I said here and discuss further there. Arınç
On 14.08.2023 16:09, Andrew Lunn wrote: >> Ah okay. I didn't consider the switch architecture where the data interface >> of the PHY is connected to the switch, and the PHY management interface is >> connected to the mdio bus that the switch is connected to. > > The generic Linux architecture for PHYs and binding them to a MAC via > a phandle allows the PHY to be on any MDIO bus anywhere. DSA has some > additional shortcuts to support 1:1 mapping if the switch has its own > MDIO bus, without describing it in DT, but this is just in addition to > the generic code. Understood. > >> Not json-schema documentation, don't care about: >> - ar9331.txt >> - lan9303.txt >> - lantiq-gswip.txt >> - marvell.txt > > The marvell switch can have up to 2 MDIO busses. If i remember > correctly, there is also one switch which has one MDIO bus per port. I will work on writing a schema for these once I'm done with this. Arınç
On 21.08.2023 20:44, Rob Herring wrote: > On Sat, Aug 12, 2023 at 10:20:43PM +0300, Arınç ÜNAL wrote: >> I've realised there are more schemas that extend the mdio.yaml schema. This >> is the final state of this patch. >> >> dt-bindings: net: dsa: document internal MDIO bus >> >> Add the schema to document the internal MDIO bus. Require the phy-handle >> property on the non-CPU ports if the mdio property is being used. >> >> Define the mdio property on all of the schemas that refer to >> dsa.yaml#/$defs/ethernet-ports. Refer to dsa.yaml#/properties/mdio to point >> the human readers to the description on the dsa.yaml schema. >> >> Some of these schemas extend the mdio.yaml schema. The mdio.yaml schema is >> also being referred to through dsa.yaml#/$defs/ethernet-ports now which >> means we cannot disallow additional properties by 'unevaluatedProperties: >> false' on the dsa.yaml schema. >> >> --- >> .../bindings/net/dsa/arrow,xrs700x.yaml | 4 ++++ >> .../devicetree/bindings/net/dsa/brcm,b53.yaml | 4 ++++ >> .../devicetree/bindings/net/dsa/brcm,sf2.yaml | 4 ++++ >> .../devicetree/bindings/net/dsa/dsa.yaml | 18 ++++++++++++++++++ >> .../bindings/net/dsa/hirschmann,hellcreek.yaml | 4 ++++ >> .../bindings/net/dsa/mediatek,mt7530.yaml | 4 ++++ >> .../bindings/net/dsa/microchip,ksz.yaml | 4 ++++ >> .../bindings/net/dsa/microchip,lan937x.yaml | 2 +- >> .../bindings/net/dsa/mscc,ocelot.yaml | 4 ++++ >> .../bindings/net/dsa/nxp,sja1105.yaml | 4 ++++ >> .../devicetree/bindings/net/dsa/qca8k.yaml | 2 +- >> .../devicetree/bindings/net/dsa/realtek.yaml | 2 +- >> .../bindings/net/dsa/renesas,rzn1-a5psw.yaml | 2 +- >> 13 files changed, 54 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml >> index 9565a740214629..f0229352e05694 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml >> @@ -29,6 +29,10 @@ properties: >> reg: >> maxItems: 1 >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> required: >> - compatible >> - reg >> diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml >> index 4c78c546343f5e..e14562b33bfb97 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml >> @@ -65,6 +65,10 @@ properties: >> - brcm,bcm63268-switch >> - const: brcm,bcm63xx-switch >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> required: >> - compatible >> - reg >> diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml >> index c745407f2f6853..1bf4317e038687 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml >> @@ -90,6 +90,10 @@ properties: >> tags enabled (per-packet metadata) >> type: boolean >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> required: >> - reg >> - interrupts >> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> index ec74a660bedaed..03ccedbc49dcc3 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> @@ -31,6 +31,24 @@ properties: >> (single device hanging off a CPU port) must not specify this property >> $ref: /schemas/types.yaml#/definitions/uint32-array >> + mdio: >> + description: The internal MDIO bus of the switch >> + $ref: /schemas/net/mdio.yaml# >> + >> +if: >> + required: [ mdio ] >> +then: >> + patternProperties: >> + "^(ethernet-)?ports$": >> + patternProperties: >> + "^(ethernet-)?port@[0-9]+$": >> + if: >> + not: >> + required: [ ethernet ] >> + then: >> + required: >> + - phy-handle >> + >> additionalProperties: true >> $defs: >> diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml >> index 4021b054f68446..32f17345825d4a 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml >> @@ -67,6 +67,10 @@ properties: >> additionalProperties: false >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> required: >> - compatible >> - reg >> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >> index e532c6b795f4fc..293d1affe75451 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >> @@ -151,6 +151,10 @@ properties: >> ethsys. >> maxItems: 1 >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> patternProperties: >> "^(ethernet-)?ports$": >> type: object >> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml >> index e51be1ac036237..01d11c642ecfd4 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml >> @@ -49,6 +49,10 @@ properties: >> Set if the output SYNCLKO clock should be disabled. Do not mix with >> microchip,synclko-125. >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> required: >> - compatible >> - reg >> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml >> index 49af4b0d591695..15f24a1716cd44 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml >> @@ -32,7 +32,7 @@ properties: >> maxItems: 1 >> mdio: >> - $ref: /schemas/net/mdio.yaml# >> + $ref: dsa.yaml#/properties/mdio >> unevaluatedProperties: false >> patternProperties: >> diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml >> index fe02d05196e4a6..d781b8c2324836 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml >> @@ -73,6 +73,10 @@ properties: >> little-endian: true >> big-endian: true >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> required: >> - compatible >> - reg >> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> index 4d5f5cc6d031e2..82dda8fae8b16e 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml >> @@ -72,6 +72,10 @@ properties: >> - compatible >> - reg >> + mdio: >> + $ref: dsa.yaml#/properties/mdio >> + unevaluatedProperties: false >> + >> patternProperties: >> "^(ethernet-)?ports$": >> patternProperties: >> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml >> index df64eebebe1856..001b72bcd0746b 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml >> @@ -60,7 +60,7 @@ properties: >> B68 on the QCA832x and B49 on the QCA833x. >> mdio: >> - $ref: /schemas/net/mdio.yaml# >> + $ref: dsa.yaml#/properties/mdio >> unevaluatedProperties: false > > Just from a schema standpoint, this is pointless indirection as > dsa.yaml#/properties/mdio is just a reference to /schemas/net/mdio.yaml#. Sure, this is only to point the human readers to the description on the dsa.yaml schema which describes the property as the internal MDIO bus of an ethernet switch. Let me know if you find this unnecessary. > > As it seems an MDIO bus is not universal for DSA, it seems you'll be > dropping this change anyways. For now, I don't think that'll be the case. Arınç
Hi Arınç, I am on vacation and I will just reply with some clarification aspects, without having done any further research on the topic since my last reply. On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote: > Before I continue commenting, I'd like to state my understanding so we can > make sure we're on the same page. If a driver doesn't use > ds->slave_mii_bus, the switch it controls must not have any internal MDIO > buses. Otherwise the PHYs on these buses couldn't function, and an improper > driver like this would not be on the official Linux source code. A DSA switch port, like any OF-based ethernet-controller which uses phylink, will use one of the phy-handle, fixed-link or managed properties to describe the interface connecting the MAC/MAC-side PCS to the PHY. At its core, ds->slave_mii_bus is nothing more than a mechanism to make sense of device trees where the above 3 phylink properties are not present. It is completely false to say that if a driver doesn't have ds->slave_mii_bus, it must not have an internal MDIO bus. Because you could still describe that internal MDIO bus like below, without making any use of the sole property that makes ds->slave_mii_bus useful from a dt-bindings perspective. ethernet-switch { ethernet-ports { port@0 { reg = <0>; phy-handle = <&port0_phy>; phy-mode = "internal"; }; }; mdio { port0_phy: ethernet-phy@0 { reg = <0>; }; }; }; This is the more universal way of describing the port setup in an OF-based way. There is also the DSA-specific (and old-style, before phylink) way of describing the same thing, which relies on the non-OF-based ds->slave_mii_bus, with bindings that look like this: ethernet-switch { ethernet-ports { port@0 { reg = <0>; }; }; }; But, I would say that the first variant of the binding is preferable, since it is more universal. Not all switches that have an internal MDIO bus support the second variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't). But, they support the same configuration through the first form. Furthermore, on the U-Boot mailing lists, I have been suggesting that the DM_DSA driver for mv88e6xxx should not bother to support the second version of the binding, since it is just more code to be added to handle a case which can already be described with the more universal first binding. > I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC > with a switch component. Since the switch component is not designed to be a > standalone IC, the MDIO bus of the SoC could just as well be used without > the need to design an MDIO controller specific to the switch component, to > manage the PHYs. So I see this switch as just another case of a switch > without an internal MDIO bus. Well, we need to clarify the semantics of an "internal" MDIO bus. I would say most discrete chips with DSA switches have this SoC-style architecture, with separate address spaces for the switching IP, MDIO bus, GPIO controller, IRQ controllers, temperature sensors etc (see "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is controlled over SPIO instead of MMIO). The dt-bindings of most DSA switches may or may not reflect that discrete chip organization. Those drivers and dt-bindings could be reimagined so that DSA is not the top-level driver. Yet, I would argue that it's wrong to say that because it isn't an OF child of the switch, the MDIO bus of the VSC7514 is not internal in the same way that the Realtek MDIO bus is internal. The switch ports are connected to internal PHYs on this MDIO bus, and there aren't PHYs on this MDIO bus that go to other MACs than the switch ports. So, the VSC7514 MDIO bus could legally be called the internal MDIO bus of the switch, even if there isn't a parent/child relationship between them. So, what I'm disagreeing with is your insistence to correlate your problem with internal MDIO buses. The way in which the problem is formulated dictates what problem gets solved, and the problem is not correctly formulated here. It is purely about ds->slave_mii_bus and its driver-defined OF presence/absence. It is a DSA-specific binding aspect which not even all DSA switches inherit, let alone bindings outside DSA. > > For switches in the second category, it all depends on the way in which > > the driver finds the node for of_mdiobus_register(). > > Ok, so some drivers require the mdio child node. Some require it and the > compatible property with a certain string. > > MDIO controlled Realtek switches do not need the compatible property under > the mdio child node. There're no compatible strings to make a distinction > between the SMI and MDIO controlled switches so the best we can do is keep > it the way it currently is. Define realtek,smi-mdio as a compatible string > but keep the compatible property optional. I did state this on my reply to > patch 3 but still received reviewed-bys regardless. Yes, because.... [1] > > Having identified all switches which make some sort of use of > > ds->slave_mii_bus, the rule would sound like this: > > > > 1. If the schema is that of (need to replace this with compatible > > strings, I'm too lazy for that): > > > > - ksz_switch_ops > > - mv88e6060_switch_ops > > - lan9303_switch_ops > > - rtl8365mb_switch_ops_mdio > > - b53_switch_ops > > - vsc73xx_ds_ops > > - mv88e6xxx > > - qca8k > > > > and we have an "mdio" child, then phylink bindings are mandatory on user ports. > > > > 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", > > then phylink bindings are mandatory on user ports (I haven't checked, > > but it might be that the "lantiq,xrx200-mdio" child is mandatory, and > > in that case, this goes to category 4 below). > > > > 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of > > "realtek,smi-mdio", then phylink bindings are mandatory on user ports > > (same comment about the child MDIO note maybe being mandatory). > > > > 4. If the switch didn't appear in the above set of rules, then phylink > > bindings are unconditionally mandatory on user ports. > > > > We don't care at all what the drivers that don't use ds->slave_mii_bus > > do with the "mdio" child node. It doesn't change the fact that their > > user ports can't have missing phylink bindings. > > I partially agree. I say, for the switches without an internal MDIO bus, > invalidate the mdio child node, and enforce the phylink bindings on the > user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x, > invalidate the mdio child node, and enforce the phylink bindings on the > user ports if the mdios property is used. Why "if the mdios property is used" and not "always"? :-/ To say it again: because the sja1105 driver does not use ds->slave_mii_bus, it can make no sense of dt-bindings on user ports which lack phylink properties. So they are *always* needed. The "mdios" property changes nothing in that regard. > > I'd like to add this before I conclude. The way I understand dt-bindings is > that a binding does not have to translate to an action on the driver. > Documenting bindings for the sole purpose of describing hardware is a valid > case. [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are otherwise the same, right? So why would they have different dt-bindings? > For example, currently, the MT753X DSA subdriver won't, in any way, > register the bus OF-based. Still, the mdio property for the switches which > this driver controls can be documented because the internal mdio bus does > exist on the hardware. It can, but the whole point is: if ds->slave_mii_bus gains an OF presence, then it loses its core functionality (that user ports can lack phylink bindings). This is the entire essence of what this discussion should capture. > > So I'd like to keep the mdio property valid for the switches which their > drivers can only register non-OF-based ds->slave_mii_bus. > > In conclusion, what to do: > > - Define "the mdio property" and "the enforcement of phylink bindings for > user ports if mdio property is used" on ethernet-switch.yaml. > - Invalidate the mdio property on the switches without an internal MDIO > bus. > - Define "the enforcement of phylink bindings for user ports" on the > switches without an internal MDIO bus. > - Require "the mdio property" for the switches which their driver requires > it to function. > - Require "the mdio property" and "the compatible string of the mdio > property" for the switches which their driver requires them to function. > > There's no 1:1 switch to switch compatible string relation, as seen on > Realtek switches so I'll have to figure that out as I go. > > I'm open to your comments to this mail but the gap between discussion and > end result has widened a lot on this patch series so I'd like to first > offload this conversation by preparing v2 with what I said here and discuss > further there. Honestly, from my side, a verbal comment in the dt-bindings document would have been just fine, as long as it is truthful to the reality it describes. You wanted to over-complicate things with an actual schema validation, and then hooking onto things that are unrelated with the phenomenon that needs to be captured (like the "mdio" child node, without explicit regard to whether it is the ds->slave_mii_bus or not). It's not about internal MDIO buses in general, it's about whether those internal MDIO buses are used in ds->slave_mii_bus, and their OF presence/absence! That is absolutely driver-specific and I would only expect a driver-specific way of enforcing it. I didn't say it's not hard, and I didn't ask to enforce it, either.
Hey Vladimir, On 27.08.2023 15:12, Vladimir Oltean wrote: > Hi Arınç, > > I am on vacation and I will just reply with some clarification aspects, > without having done any further research on the topic since my last reply. > > On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote: >> Before I continue commenting, I'd like to state my understanding so we can >> make sure we're on the same page. If a driver doesn't use >> ds->slave_mii_bus, the switch it controls must not have any internal MDIO >> buses. Otherwise the PHYs on these buses couldn't function, and an improper >> driver like this would not be on the official Linux source code. > > A DSA switch port, like any OF-based ethernet-controller which uses > phylink, will use one of the phy-handle, fixed-link or managed properties > to describe the interface connecting the MAC/MAC-side PCS to the PHY. > > At its core, ds->slave_mii_bus is nothing more than a mechanism to make > sense of device trees where the above 3 phylink properties are not present. > > It is completely false to say that if a driver doesn't have ds->slave_mii_bus, > it must not have an internal MDIO bus. Because you could still describe > that internal MDIO bus like below, without making any use of the sole property > that makes ds->slave_mii_bus useful from a dt-bindings perspective. > > ethernet-switch { > ethernet-ports { > port@0 { > reg = <0>; > phy-handle = <&port0_phy>; > phy-mode = "internal"; > }; > }; > > mdio { > port0_phy: ethernet-phy@0 { > reg = <0>; > }; > }; > }; > > This is the more universal way of describing the port setup in an > OF-based way. There is also the DSA-specific (and old-style, before phylink) > way of describing the same thing, which relies on the non-OF-based > ds->slave_mii_bus, with bindings that look like this: > > ethernet-switch { > ethernet-ports { > port@0 { > reg = <0>; > }; > }; > }; > > But, I would say that the first variant of the binding is preferable, > since it is more universal. > > Not all switches that have an internal MDIO bus support the second > variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't). > But, they support the same configuration through the first form. Understood. > > Furthermore, on the U-Boot mailing lists, I have been suggesting that > the DM_DSA driver for mv88e6xxx should not bother to support the second > version of the binding, since it is just more code to be added to handle > a case which can already be described with the more universal first binding. That makes sense. > >> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC >> with a switch component. Since the switch component is not designed to be a >> standalone IC, the MDIO bus of the SoC could just as well be used without >> the need to design an MDIO controller specific to the switch component, to >> manage the PHYs. So I see this switch as just another case of a switch >> without an internal MDIO bus. > > Well, we need to clarify the semantics of an "internal" MDIO bus. > > I would say most discrete chips with DSA switches have this SoC-style > architecture, with separate address spaces for the switching IP, MDIO > bus, GPIO controller, IRQ controllers, temperature sensors etc (see > "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is > controlled over SPIO instead of MMIO). The dt-bindings of most DSA > switches may or may not reflect that discrete chip organization. Those > drivers and dt-bindings could be reimagined so that DSA is not the > top-level driver. > > Yet, I would argue that it's wrong to say that because it isn't an OF > child of the switch, the MDIO bus of the VSC7514 is not internal in the > same way that the Realtek MDIO bus is internal. The switch ports are > connected to internal PHYs on this MDIO bus, and there aren't PHYs on > this MDIO bus that go to other MACs than the switch ports. So, the > VSC7514 MDIO bus could legally be called the internal MDIO bus of the > switch, even if there isn't a parent/child relationship between them. Good point, I had believed that the management interface of all of the PHYs being connected to the MDIO bus - which is not part of the switching IP address space - would be enough to classify the MDIO bus as non-internal. However, the architecture of separate address spaces for the switching IP and MDIO bus is used on any type of IC with the switching feature. Therefore, this characteristic cannot be used to distinguish whether an MDIO bus is of a switch. What we can refer to to classify an internal MDIO bus is by confirming the data interface of all PHYs on the MDIO bus is connected to the switch port MACs, as you have pointed out here. Because the architecture of separate address spaces for the switching IP and MDIO bus is used on any type of IC with the switching feature, it can differ by driver how the MDIO bus is defined on the dt-bindings. So we can't make universal bindings of an internal MDIO bus of a switch that apply to every switch. So, the correct approach is to define things under the switch-specific schema which is affine to the driver, as you have already pointed out. Which schemas to define what will of course differ. > > So, what I'm disagreeing with is your insistence to correlate your > problem with internal MDIO buses. The way in which the problem is > formulated dictates what problem gets solved, and the problem is not > correctly formulated here. It is purely about ds->slave_mii_bus and its > driver-defined OF presence/absence. It is a DSA-specific binding aspect > which not even all DSA switches inherit, let alone bindings outside DSA. Got it. > >>> For switches in the second category, it all depends on the way in which >>> the driver finds the node for of_mdiobus_register(). >> >> Ok, so some drivers require the mdio child node. Some require it and the >> compatible property with a certain string. >> >> MDIO controlled Realtek switches do not need the compatible property under >> the mdio child node. There're no compatible strings to make a distinction >> between the SMI and MDIO controlled switches so the best we can do is keep >> it the way it currently is. Define realtek,smi-mdio as a compatible string >> but keep the compatible property optional. I did state this on my reply to >> patch 3 but still received reviewed-bys regardless. > > Yes, because.... [1] > >>> Having identified all switches which make some sort of use of >>> ds->slave_mii_bus, the rule would sound like this: >>> >>> 1. If the schema is that of (need to replace this with compatible >>> strings, I'm too lazy for that): >>> >>> - ksz_switch_ops >>> - mv88e6060_switch_ops >>> - lan9303_switch_ops >>> - rtl8365mb_switch_ops_mdio >>> - b53_switch_ops >>> - vsc73xx_ds_ops >>> - mv88e6xxx >>> - qca8k >>> >>> and we have an "mdio" child, then phylink bindings are mandatory on user ports. >>> >>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", >>> then phylink bindings are mandatory on user ports (I haven't checked, >>> but it might be that the "lantiq,xrx200-mdio" child is mandatory, and >>> in that case, this goes to category 4 below). >>> >>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of >>> "realtek,smi-mdio", then phylink bindings are mandatory on user ports >>> (same comment about the child MDIO note maybe being mandatory). >>> >>> 4. If the switch didn't appear in the above set of rules, then phylink >>> bindings are unconditionally mandatory on user ports. >>> >>> We don't care at all what the drivers that don't use ds->slave_mii_bus >>> do with the "mdio" child node. It doesn't change the fact that their >>> user ports can't have missing phylink bindings. >> >> I partially agree. I say, for the switches without an internal MDIO bus, >> invalidate the mdio child node, and enforce the phylink bindings on the >> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x, >> invalidate the mdio child node, and enforce the phylink bindings on the >> user ports if the mdios property is used. > > Why "if the mdios property is used" and not "always"? :-/ > > To say it again: because the sja1105 driver does not use ds->slave_mii_bus, > it can make no sense of dt-bindings on user ports which lack phylink properties. > So they are *always* needed. The "mdios" property changes nothing in that regard. Got it. > >> >> I'd like to add this before I conclude. The way I understand dt-bindings is >> that a binding does not have to translate to an action on the driver. >> Documenting bindings for the sole purpose of describing hardware is a valid >> case. > > [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are > otherwise the same, right? So why would they have different dt-bindings? Honestly, I'm wondering the answer to this as well. For some reason, when probing the SMI controlled Realtek switches, instead of just letting dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() on realtek-smi.c: - priv->slave_mii_bus is allocated. - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); - priv->slave_mii_bus->dev.of_node = mdio_np; - ds->slave_mii_bus = priv->slave_mii_bus; > >> For example, currently, the MT753X DSA subdriver won't, in any way, >> register the bus OF-based. Still, the mdio property for the switches which >> this driver controls can be documented because the internal mdio bus does >> exist on the hardware. > > It can, but the whole point is: if ds->slave_mii_bus gains an OF presence, > then it loses its core functionality (that user ports can lack phylink > bindings). This is the entire essence of what this discussion should capture. Understood. > >> >> So I'd like to keep the mdio property valid for the switches which their >> drivers can only register non-OF-based ds->slave_mii_bus. >> >> In conclusion, what to do: >> >> - Define "the mdio property" and "the enforcement of phylink bindings for >> user ports if mdio property is used" on ethernet-switch.yaml. >> - Invalidate the mdio property on the switches without an internal MDIO >> bus. >> - Define "the enforcement of phylink bindings for user ports" on the >> switches without an internal MDIO bus. >> - Require "the mdio property" for the switches which their driver requires >> it to function. >> - Require "the mdio property" and "the compatible string of the mdio >> property" for the switches which their driver requires them to function. >> >> There's no 1:1 switch to switch compatible string relation, as seen on >> Realtek switches so I'll have to figure that out as I go. >> >> I'm open to your comments to this mail but the gap between discussion and >> end result has widened a lot on this patch series so I'd like to first >> offload this conversation by preparing v2 with what I said here and discuss >> further there. > > Honestly, from my side, a verbal comment in the dt-bindings document > would have been just fine, as long as it is truthful to the reality it > describes. > > You wanted to over-complicate things with an actual schema validation, > and then hooking onto things that are unrelated with the phenomenon that > needs to be captured (like the "mdio" child node, without explicit > regard to whether it is the ds->slave_mii_bus or not). > > It's not about internal MDIO buses in general, it's about whether those > internal MDIO buses are used in ds->slave_mii_bus, and their OF > presence/absence! That is absolutely driver-specific and I would only > expect a driver-specific way of enforcing it. I didn't say it's not > hard, and I didn't ask to enforce it, either. OK, I believe we're on the same page now, I will start working on properly enforcing this. Arınç
> > [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are > > otherwise the same, right? So why would they have different dt-bindings? > > Honestly, I'm wondering the answer to this as well. For some reason, when > probing the SMI controlled Realtek switches, instead of just letting > dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() > on realtek-smi.c: > > - priv->slave_mii_bus is allocated. > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); > - priv->slave_mii_bus->dev.of_node = mdio_np; > - ds->slave_mii_bus = priv->slave_mii_bus; I might be able to help here. The Realtek SMI version created a custom slave_mii driver because it was the only way to associate it with an MDIO DT node. And that DT node was required to specify the interrupts for each phy0. It would work without that mdio node, letting DSA setup handle the slave bus, but it would rely only on polling for port status. As we only have a single internal MDIO, the compatible string "realtek,smi-mdio" would not be necessary if the driver checks for a "mdio"-named child node. Maybe the code was just inspired by another DSA driver that uses more MDIO buses or external ones. The "mdio" name is suggested by docs since it was committed (https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt). That name was also kept in the YAML translation (https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek.yaml). The Realtek MDIO driver was merged at the same release that included the change that allows dsa_switch_setup() to reference the "mdio" OF-node if present. That way, it could avoid creating a custom slave_mii_bus driver. I submitted a small series of patches to unify that behavior between those two drivers: https://lore.kernel.org/netdev/CAJq09z44SNGFkCi_BCpQ+3DuXhKfGVsMubRYE7AezJsGGOboVA@mail.gmail.com/ (This is my answer to the series opening message to include the first paragraph ate by the editor) There was some discussion but not NAC, ACK or RFC. It would have dropped some lines of code. I can revive it if there is interest. Regards, Luiz
On 5.09.2023 05:42, Luiz Angelo Daros de Luca wrote: >>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are >>> otherwise the same, right? So why would they have different dt-bindings? >> >> Honestly, I'm wondering the answer to this as well. For some reason, when >> probing the SMI controlled Realtek switches, instead of just letting >> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() >> on realtek-smi.c: >> >> - priv->slave_mii_bus is allocated. >> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); >> - priv->slave_mii_bus->dev.of_node = mdio_np; >> - ds->slave_mii_bus = priv->slave_mii_bus; > > I might be able to help here. The Realtek SMI version created a custom > slave_mii driver because it was the only way to associate it with an > MDIO DT node. And that DT node was required to specify the interrupts > for each phy0. > It would work without that mdio node, letting DSA setup handle the > slave bus, but it would rely only on polling for port status. > > As we only have a single internal MDIO, the compatible string > "realtek,smi-mdio" would not be necessary if the driver checks for a > "mdio"-named child node. Maybe the code was just inspired by another > DSA driver that uses more MDIO buses or external ones. The "mdio" name > is suggested by docs since it was committed > (https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt). > That name was also kept in the YAML translation > (https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/realtek.yaml). > > The Realtek MDIO driver was merged at the same release that included > the change that allows dsa_switch_setup() to reference the "mdio" > OF-node if present. That way, it could avoid creating a custom > slave_mii_bus driver. > > I submitted a small series of patches to unify that behavior between > those two drivers: > > https://lore.kernel.org/netdev/CAJq09z44SNGFkCi_BCpQ+3DuXhKfGVsMubRYE7AezJsGGOboVA@mail.gmail.com/ > (This is my answer to the series opening message to include the first > paragraph ate by the editor) > > There was some discussion but not NAC, ACK or RFC. It would have > dropped some lines of code. I can revive it if there is interest. I'd like this to happen, thanks Luiz! Arınç
On Mon, Sep 04, 2023 at 11:42:19PM -0300, Luiz Angelo Daros de Luca wrote: > I submitted a small series of patches to unify that behavior between > those two drivers: > > https://lore.kernel.org/netdev/CAJq09z44SNGFkCi_BCpQ+3DuXhKfGVsMubRYE7AezJsGGOboVA@mail.gmail.com/ > (This is my answer to the series opening message to include the first > paragraph ate by the editor) > > There was some discussion but not NAC, ACK or RFC. It would have > dropped some lines of code. I can revive it if there is interest. There was no Ack or Nack from me because I didn't manage to understand what bothers you if the unified dt-binding has a "compatible" string for the "mdio" node, which the driver simply ignores. https://lore.kernel.org/netdev/20220706152923.mhc7vw7xkr7xkot4@skbuf/
On Mon, Sep 04, 2023 at 11:42:19PM -0300, Luiz Angelo Daros de Luca wrote: > > > [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are > > > otherwise the same, right? So why would they have different dt-bindings? > > > > Honestly, I'm wondering the answer to this as well. For some reason, when > > probing the SMI controlled Realtek switches, instead of just letting > > dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() > > on realtek-smi.c: > > > > - priv->slave_mii_bus is allocated. > > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); > > - priv->slave_mii_bus->dev.of_node = mdio_np; > > - ds->slave_mii_bus = priv->slave_mii_bus; > > I might be able to help here. The Realtek SMI version created a custom > slave_mii driver because it was the only way to associate it with an > MDIO DT node. And that DT node was required to specify the interrupts > for each phy0. > It would work without that mdio node, letting DSA setup handle the > slave bus, but it would rely only on polling for port status. It is possible to set up PHY IRQs even if the MDIO bus is not OF-based. I think that mv88e6xxx_g2_irq_mdio_setup() does that (sets bus->irq[]).
> It is possible to set up PHY IRQs even if the MDIO bus is not OF-based. > I think that mv88e6xxx_g2_irq_mdio_setup() does that (sets bus->irq[]). Yes. It took me a while to realise you could do this, so there is probably some complexity in mv88e6xxx i might of been able to avoid if i had discovered this earlier. Andrew
Hi Andrew, On 14.08.2023 16:09, Andrew Lunn wrote: >> Ah okay. I didn't consider the switch architecture where the data interface >> of the PHY is connected to the switch, and the PHY management interface is >> connected to the mdio bus that the switch is connected to. > > The generic Linux architecture for PHYs and binding them to a MAC via > a phandle allows the PHY to be on any MDIO bus anywhere. DSA has some > additional shortcuts to support 1:1 mapping if the switch has its own > MDIO bus, without describing it in DT, but this is just in addition to > the generic code. > >> Not json-schema documentation, don't care about: >> - ar9331.txt >> - lan9303.txt >> - lantiq-gswip.txt >> - marvell.txt > > The marvell switch can have up to 2 MDIO busses. If i remember > correctly, there is also one switch which has one MDIO bus per port. I'm writing the json-schema for Marvell switches. I've checked a few devicetree source files on Linus's Linux tree, I only see two buses used at the most. The internal bus and another bus with marvell,mv88e6xxx-mdio-external. I've never seen a devicetree with marvell,mv88e6250 though. Could the switch that has one MDIO bus per port be this one? Also, is every bus of this switch a marvell,mv88e6xxx-mdio-external bus or, one internal bus and the remaining are marvell mv88e6xxx-mdio-external buses? Arınç
On 4.09.2023 14:33, Arınç ÜNAL wrote: > Hey Vladimir, > > On 27.08.2023 15:12, Vladimir Oltean wrote: >> Hi Arınç, >> >> I am on vacation and I will just reply with some clarification aspects, >> without having done any further research on the topic since my last reply. >> >> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote: >>> Before I continue commenting, I'd like to state my understanding so we can >>> make sure we're on the same page. If a driver doesn't use >>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO >>> buses. Otherwise the PHYs on these buses couldn't function, and an improper >>> driver like this would not be on the official Linux source code. >> >> A DSA switch port, like any OF-based ethernet-controller which uses >> phylink, will use one of the phy-handle, fixed-link or managed properties >> to describe the interface connecting the MAC/MAC-side PCS to the PHY. >> >> At its core, ds->slave_mii_bus is nothing more than a mechanism to make >> sense of device trees where the above 3 phylink properties are not present. >> >> It is completely false to say that if a driver doesn't have ds->slave_mii_bus, >> it must not have an internal MDIO bus. Because you could still describe >> that internal MDIO bus like below, without making any use of the sole property >> that makes ds->slave_mii_bus useful from a dt-bindings perspective. >> >> ethernet-switch { >> ethernet-ports { >> port@0 { >> reg = <0>; >> phy-handle = <&port0_phy>; >> phy-mode = "internal"; >> }; >> }; >> >> mdio { >> port0_phy: ethernet-phy@0 { >> reg = <0>; >> }; >> }; >> }; >> >> This is the more universal way of describing the port setup in an >> OF-based way. There is also the DSA-specific (and old-style, before phylink) >> way of describing the same thing, which relies on the non-OF-based >> ds->slave_mii_bus, with bindings that look like this: >> >> ethernet-switch { >> ethernet-ports { >> port@0 { >> reg = <0>; >> }; >> }; >> }; >> >> But, I would say that the first variant of the binding is preferable, >> since it is more universal. >> >> Not all switches that have an internal MDIO bus support the second >> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't). >> But, they support the same configuration through the first form. > > Understood. > >> >> Furthermore, on the U-Boot mailing lists, I have been suggesting that >> the DM_DSA driver for mv88e6xxx should not bother to support the second >> version of the binding, since it is just more code to be added to handle >> a case which can already be described with the more universal first binding. > > That makes sense. > >> >>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC >>> with a switch component. Since the switch component is not designed to be a >>> standalone IC, the MDIO bus of the SoC could just as well be used without >>> the need to design an MDIO controller specific to the switch component, to >>> manage the PHYs. So I see this switch as just another case of a switch >>> without an internal MDIO bus. >> >> Well, we need to clarify the semantics of an "internal" MDIO bus. >> >> I would say most discrete chips with DSA switches have this SoC-style >> architecture, with separate address spaces for the switching IP, MDIO >> bus, GPIO controller, IRQ controllers, temperature sensors etc (see >> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is >> controlled over SPIO instead of MMIO). The dt-bindings of most DSA >> switches may or may not reflect that discrete chip organization. Those >> drivers and dt-bindings could be reimagined so that DSA is not the >> top-level driver. >> >> Yet, I would argue that it's wrong to say that because it isn't an OF >> child of the switch, the MDIO bus of the VSC7514 is not internal in the >> same way that the Realtek MDIO bus is internal. The switch ports are >> connected to internal PHYs on this MDIO bus, and there aren't PHYs on >> this MDIO bus that go to other MACs than the switch ports. So, the >> VSC7514 MDIO bus could legally be called the internal MDIO bus of the >> switch, even if there isn't a parent/child relationship between them. > > Good point, I had believed that the management interface of all of the PHYs > being connected to the MDIO bus - which is not part of the switching IP > address space - would be enough to classify the MDIO bus as non-internal. > > However, the architecture of separate address spaces for the switching IP > and MDIO bus is used on any type of IC with the switching feature. > Therefore, this characteristic cannot be used to distinguish whether an > MDIO bus is of a switch. > > What we can refer to to classify an internal MDIO bus is by confirming the > data interface of all PHYs on the MDIO bus is connected to the switch port > MACs, as you have pointed out here. > > Because the architecture of separate address spaces for the switching IP > and MDIO bus is used on any type of IC with the switching feature, it can > differ by driver how the MDIO bus is defined on the dt-bindings. So we > can't make universal bindings of an internal MDIO bus of a switch that > apply to every switch. > > So, the correct approach is to define things under the switch-specific > schema which is affine to the driver, as you have already pointed out. > Which schemas to define what will of course differ. > >> >> So, what I'm disagreeing with is your insistence to correlate your >> problem with internal MDIO buses. The way in which the problem is >> formulated dictates what problem gets solved, and the problem is not >> correctly formulated here. It is purely about ds->slave_mii_bus and its >> driver-defined OF presence/absence. It is a DSA-specific binding aspect >> which not even all DSA switches inherit, let alone bindings outside DSA. > > Got it. > >> >>>> For switches in the second category, it all depends on the way in which >>>> the driver finds the node for of_mdiobus_register(). >>> >>> Ok, so some drivers require the mdio child node. Some require it and the >>> compatible property with a certain string. >>> >>> MDIO controlled Realtek switches do not need the compatible property under >>> the mdio child node. There're no compatible strings to make a distinction >>> between the SMI and MDIO controlled switches so the best we can do is keep >>> it the way it currently is. Define realtek,smi-mdio as a compatible string >>> but keep the compatible property optional. I did state this on my reply to >>> patch 3 but still received reviewed-bys regardless. >> >> Yes, because.... [1] >> >>>> Having identified all switches which make some sort of use of >>>> ds->slave_mii_bus, the rule would sound like this: >>>> >>>> 1. If the schema is that of (need to replace this with compatible >>>> strings, I'm too lazy for that): >>>> >>>> - ksz_switch_ops >>>> - mv88e6060_switch_ops >>>> - lan9303_switch_ops >>>> - rtl8365mb_switch_ops_mdio >>>> - b53_switch_ops >>>> - vsc73xx_ds_ops >>>> - mv88e6xxx >>>> - qca8k >>>> >>>> and we have an "mdio" child, then phylink bindings are mandatory on user ports. >>>> >>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", >>>> then phylink bindings are mandatory on user ports (I haven't checked, >>>> but it might be that the "lantiq,xrx200-mdio" child is mandatory, and >>>> in that case, this goes to category 4 below). >>>> >>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of >>>> "realtek,smi-mdio", then phylink bindings are mandatory on user ports >>>> (same comment about the child MDIO note maybe being mandatory). >>>> >>>> 4. If the switch didn't appear in the above set of rules, then phylink >>>> bindings are unconditionally mandatory on user ports. >>>> >>>> We don't care at all what the drivers that don't use ds->slave_mii_bus >>>> do with the "mdio" child node. It doesn't change the fact that their >>>> user ports can't have missing phylink bindings. >>> >>> I partially agree. I say, for the switches without an internal MDIO bus, >>> invalidate the mdio child node, and enforce the phylink bindings on the >>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x, >>> invalidate the mdio child node, and enforce the phylink bindings on the >>> user ports if the mdios property is used. >> >> Why "if the mdios property is used" and not "always"? :-/ >> >> To say it again: because the sja1105 driver does not use ds->slave_mii_bus, >> it can make no sense of dt-bindings on user ports which lack phylink properties. >> So they are *always* needed. The "mdios" property changes nothing in that regard. > > Got it. > >> >>> >>> I'd like to add this before I conclude. The way I understand dt-bindings is >>> that a binding does not have to translate to an action on the driver. >>> Documenting bindings for the sole purpose of describing hardware is a valid >>> case. >> >> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are >> otherwise the same, right? So why would they have different dt-bindings? > > Honestly, I'm wondering the answer to this as well. For some reason, when > probing the SMI controlled Realtek switches, instead of just letting > dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() > on realtek-smi.c: > > - priv->slave_mii_bus is allocated. > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); > - priv->slave_mii_bus->dev.of_node = mdio_np; > - ds->slave_mii_bus = priv->slave_mii_bus; > >> >>> For example, currently, the MT753X DSA subdriver won't, in any way, >>> register the bus OF-based. Still, the mdio property for the switches which >>> this driver controls can be documented because the internal mdio bus does >>> exist on the hardware. >> >> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence, >> then it loses its core functionality (that user ports can lack phylink >> bindings). This is the entire essence of what this discussion should capture. > > Understood. > >> >>> >>> So I'd like to keep the mdio property valid for the switches which their >>> drivers can only register non-OF-based ds->slave_mii_bus. >>> >>> In conclusion, what to do: >>> >>> - Define "the mdio property" and "the enforcement of phylink bindings for >>> user ports if mdio property is used" on ethernet-switch.yaml. >>> - Invalidate the mdio property on the switches without an internal MDIO >>> bus. >>> - Define "the enforcement of phylink bindings for user ports" on the >>> switches without an internal MDIO bus. >>> - Require "the mdio property" for the switches which their driver requires >>> it to function. >>> - Require "the mdio property" and "the compatible string of the mdio >>> property" for the switches which their driver requires them to function. >>> >>> There's no 1:1 switch to switch compatible string relation, as seen on >>> Realtek switches so I'll have to figure that out as I go. >>> >>> I'm open to your comments to this mail but the gap between discussion and >>> end result has widened a lot on this patch series so I'd like to first >>> offload this conversation by preparing v2 with what I said here and discuss >>> further there. >> >> Honestly, from my side, a verbal comment in the dt-bindings document >> would have been just fine, as long as it is truthful to the reality it >> describes. >> >> You wanted to over-complicate things with an actual schema validation, >> and then hooking onto things that are unrelated with the phenomenon that >> needs to be captured (like the "mdio" child node, without explicit >> regard to whether it is the ds->slave_mii_bus or not). >> >> It's not about internal MDIO buses in general, it's about whether those >> internal MDIO buses are used in ds->slave_mii_bus, and their OF >> presence/absence! That is absolutely driver-specific and I would only >> expect a driver-specific way of enforcing it. I didn't say it's not >> hard, and I didn't ask to enforce it, either. > > OK, I believe we're on the same page now, I will start working on properly > enforcing this. I'm writing below as a mix of patch log and discussion. Phylink bindings are required for ports that are controlled by OF-based buses. DSA, like any other driver utilising the Linux MDIO infrastructure, can register a bus. If I understand correctly, non-OF-based registration of OpenFirmware MDIO buses is a feature specific to DSA which certain DSA subdrivers make use of. There's no way to distinguish which port is controlled by which driver's MDIO bus on the bindings so we can't enforce phylink bindings for all user ports as this would also enforce phylink bindings on user ports controlled by a non-OF-based bus. But we can know when DSA won't create a non-OF-based bus. That leaves us with only OF-based buses in which case we can enforce phylink bindings for all user ports. So we need to check each DSA subdriver to see when all buses will be OF-based. A DSA subdriver can either let the main DSA driver register the bus or it can register a bus or buses itself. The attributes of a DSA subdriver that lets the DSA driver register the bus: - ds->ops->phy_read() and ds->ops->phy_write() are present. - ds->slave_mii_bus is not populated by the DSA subdriver. - The bus is registered non-OF-based or OF-based. Registered OF-based if "mdio" child node is defined. How each DSA subdriver behaves is documented below. --- - mscc,vsc7514-switch.yaml - mscc,vsc7514-switch - mscc,vsc7512-switch drivers/net/ethernet/mscc/ocelot_vsc7514.c: - Not a DSA subdriver. - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used to register a bus. drivers/net/dsa/ocelot/ocelot_ext.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used to register a bus. What to do: - For mscc,vsc7514-switch, enforce phylink bindings for ports. - For mscc,vsc7512-switch, enforce phylink bindings for user ports. --- - renesas,rzn1-a5psw.yaml - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw drivers/net/dsa/rzn1_a5psw.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Registers the bus OF-based only. Registers the bus when "mdio" child node is defined. - mdio = of_get_child_by_name(dev->of_node, "mdio"); if (of_device_is_available(mdio)) ret = a5psw_probe_mdio(a5psw, mdio); What to do: - Document "mdio". - Enforce phylink bindings for user ports. --- - realtek.yaml - realtek,rtl8365mb - realtek,rtl8366rb drivers/net/dsa/realtek/realtek-smi.c: - The DSA subdriver won't let the DSA driver register the bus. Registers the bus OF-based only. Registering the bus is mandatory. Registers the bus when compatible string "realtek,smi-mdio" on a child node is defined. - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); if (!mdio_np) return -ENODEV; ds->slave_mii_bus = priv->slave_mii_bus; drivers/net/dsa/realtek/realtek-mdio.c: - The DSA subdriver lets the DSA driver register the bus. What to do: - Document "mdio". - Require "mdio". (Can't do because it's not required for MDIO controlled switches that share the compatible string with SMI controlled switches. This is why I would like Luiz to unify the bus registeration process.) - Document compatible string "realtek,smi-mdio" on "mdio" child node. - Require compatible. (Can't do because the same as above.) - Enforce phylink bindings for user ports. (Can't do because the same as above.) - Enforce phylink bindings for user ports if "mdio" is defined. --- - qca8k.yaml - qca,qca8327 - qca,qca8328 - qca,qca8334 - qca,qca8337 drivers/net/dsa/qca/qca8k-8xxx.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" child node is defined. - mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. --- - nxp,sja1105.yaml - nxp,sja1105e - nxp,sja1105t - nxp,sja1105p - nxp,sja1105q - nxp,sja1105r - nxp,sja1105s - nxp,sja1110a - nxp,sja1110b - nxp,sja1110c - nxp,sja1110d drivers/net/dsa/sja1105/sja1105_mdio.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Registers multiple buses OF-based only. Registers the buses when "mdios" child node and "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio" compatible strings per child node under "mdios" is defined. - mdio_node = of_get_child_by_name(switch_node, "mdios"); if (!mdio_node) return 0; - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio"); if (!np) return 0; - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio"); if (!np) return 0; What to do: - Document "mdios". - Document child node pattern property under "mdios". - Document "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio" compatible strings. - Enforce phylink bindings for user ports. --- - mscc,ocelot.yaml - mscc,vsc9953-switch - pci1957,eef0 drivers/net/dsa/ocelot/seville_vsc9953.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used to register a bus. drivers/net/dsa/ocelot/felix_vsc9959.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - FSL_ENETC_MDIO, a driver utilising the Linux MDIO infrastructure is used to register a bus. What to do: - Enforce phylink bindings for user ports. --- - microchip,lan937x.yaml - microchip,lan9370 - microchip,lan9371 - microchip,lan9372 - microchip,lan9373 - microchip,lan9374 - microchip,ksz.yaml - microchip,ksz8765 - microchip,ksz8794 - microchip,ksz8795 - microchip,ksz8863 - microchip,ksz8873 - microchip,ksz9477 - microchip,ksz9897 - microchip,ksz9896 - microchip,ksz9567 - microchip,ksz8565 - microchip,ksz9893 - microchip,ksz9563 - microchip,ksz8563 drivers/net/dsa/microchip/ksz_common.c: - The DSA subdriver won't let the DSA driver register the bus when "mdio" child node is defined. Registers the bus OF-based only. Registers the bus when "mdio" child node is defined. - mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio"); if (!mdio_np) return 0; ds->slave_mii_bus = bus; What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. --- - hirschmann,hellcreek.yaml - hirschmann,hellcreek-de1soc-r1 drivers/net/dsa/hirschmann/hellcreek.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). What to do: - Enforce phylink bindings for user ports. --- - mediatek,mt7530.yaml - mediatek,mt7530 - mediatek,mt7531 - mediatek,mt7621 - mediatek,mt7988-switch drivers/net/dsa/mt7530.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" child node is defined. (This is after the patch for the MT7530 DSA subdriver is applied.) What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. --- - brcm,sf2.yaml - brcm,bcm4908-switch - brcm,bcm7278-switch-v4.0 - brcm,bcm7278-switch-v4.8 - brcm,bcm7445-switch-v4.0 drivers/net/dsa/bcm_sf2.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Requires MDIO_BCM_UNIMAC, a driver utilising the Linux MDIO infrastructure to register a bus. - dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio"); priv->master_mii_bus = of_mdio_find_bus(dn); if (!priv->master_mii_bus) { of_node_put(dn); return -EPROBE_DEFER; } What to do: - Enforce phylink bindings for user ports. --- - brcm,b53.yaml - brcm,bcm5325 - brcm,bcm53115 - brcm,bcm53125 - brcm,bcm53128 - brcm,bcm53134 - brcm,bcm5365 - brcm,bcm5395 - brcm,bcm5389 - brcm,bcm5397 - brcm,bcm5398 - brcm,bcm11360-srab, brcm,cygnus-srab - brcm,bcm53010-srab, brcm,bcm5301x-srab - brcm,bcm53011-srab, brcm,bcm5301x-srab - brcm,bcm53012-srab, brcm,bcm5301x-srab - brcm,bcm53018-srab, brcm,bcm5301x-srab - brcm,bcm53019-srab, brcm,bcm5301x-srab - brcm,bcm11404-srab, brcm,omega-srab - brcm,bcm11407-srab, brcm,omega-srab - brcm,bcm11409-srab, brcm,omega-srab - brcm,bcm58310-srab, brcm,omega-srab - brcm,bcm58311-srab, brcm,omega-srab - brcm,bcm58313-srab, brcm,omega-srab - brcm,bcm58522-srab, brcm,nsp-srab - brcm,bcm58523-srab, brcm,nsp-srab - brcm,bcm58525-srab, brcm,nsp-srab - brcm,bcm58622-srab, brcm,nsp-srab - brcm,bcm58623-srab, brcm,nsp-srab - brcm,bcm58625-srab, brcm,nsp-srab - brcm,bcm88312-srab, brcm,nsp-srab - brcm,bcm3384-switch, brcm,bcm63xx-switch - brcm,bcm6318-switch, brcm,bcm63xx-switch - brcm,bcm6328-switch, brcm,bcm63xx-switch - brcm,bcm6362-switch, brcm,bcm63xx-switch - brcm,bcm6368-switch, brcm,bcm63xx-switch - brcm,bcm63268-switch, brcm,bcm63xx-switch drivers/net/dsa/b53/b53_common.c: - The DSA subdriver lets the DSA driver register the bus. What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. --- - arrow,xrs700x.yaml - arrow,xrs7003e - arrow,xrs7003f - arrow,xrs7004e - arrow,xrs7004f drivers/net/dsa/xrs700x/xrs700x.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). What to do: - Enforce phylink bindings for user ports. --- Text documents. I will handle these when I replace them with json-schema documents. - ar9331.txt - qca,ar9331-switch drivers/net/dsa/qca/ar9331.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Registers the bus OF-based only. Registering the bus is mandatory. Registers the bus when "mdio" child node is defined. - mnp = of_get_child_by_name(np, "mdio"); if (!mnp) return -ENODEV; What to do: - Document "mdio". - Require "mdio". - Enforce phylink bindings for user ports. --- - lan9303.txt - smsc,lan9303-i2c - smsc,lan9303-mdio drivers/net/dsa/lan9303-core.c: - The DSA subdriver lets the DSA driver register the bus. What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. --- - lantiq-gswip.txt - lantiq,xrx200-gswip - lantiq,xrx300-gswip - lantiq,xrx330-gswip drivers/net/dsa/lantiq_gswip.c: - The DSA subdriver won't let the DSA driver register the bus. - No ds->ops->phy_read() or ds->ops->phy_write(). - Registers the bus OF-based only. Registers the bus when compatible string "lantiq,xrx200-mdio" on a child node is defined. - mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio"); if (mdio_np) err = gswip_mdio(priv, mdio_np); What to do: - Document "mdio". - Document compatible string "realtek,smi-mdio" on "mdio" child node. - Require compatible. - Enforce phylink bindings for user ports. --- - marvell.txt - marvell,mv88e6085 - marvell,mv88e6190 - marvell,mv88e6250 drivers/net/dsa/mv88e6xxx/chip.c: - The DSA subdriver won't let the DSA driver register the bus. - ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip); - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" child node is defined. What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. --- - vitesse,vsc73xx.txt - vitesse,vsc7385 - vitesse,vsc7388 - vitesse,vsc7395 - vitesse,vsc7398 drivers/net/dsa/vitesse-vsc73xx-core.c: - The DSA subdriver lets the DSA driver register the bus. What to do: - Document "mdio". - Enforce phylink bindings for user ports if "mdio" is defined. Arınç
On 9.09.2023 11:53, Arınç ÜNAL wrote: > On 4.09.2023 14:33, Arınç ÜNAL wrote: >> Hey Vladimir, >> >> On 27.08.2023 15:12, Vladimir Oltean wrote: >>> Hi Arınç, >>> >>> I am on vacation and I will just reply with some clarification aspects, >>> without having done any further research on the topic since my last reply. >>> >>> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote: >>>> Before I continue commenting, I'd like to state my understanding so we can >>>> make sure we're on the same page. If a driver doesn't use >>>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO >>>> buses. Otherwise the PHYs on these buses couldn't function, and an improper >>>> driver like this would not be on the official Linux source code. >>> >>> A DSA switch port, like any OF-based ethernet-controller which uses >>> phylink, will use one of the phy-handle, fixed-link or managed properties >>> to describe the interface connecting the MAC/MAC-side PCS to the PHY. >>> >>> At its core, ds->slave_mii_bus is nothing more than a mechanism to make >>> sense of device trees where the above 3 phylink properties are not present. >>> >>> It is completely false to say that if a driver doesn't have ds->slave_mii_bus, >>> it must not have an internal MDIO bus. Because you could still describe >>> that internal MDIO bus like below, without making any use of the sole property >>> that makes ds->slave_mii_bus useful from a dt-bindings perspective. >>> >>> ethernet-switch { >>> ethernet-ports { >>> port@0 { >>> reg = <0>; >>> phy-handle = <&port0_phy>; >>> phy-mode = "internal"; >>> }; >>> }; >>> >>> mdio { >>> port0_phy: ethernet-phy@0 { >>> reg = <0>; >>> }; >>> }; >>> }; >>> >>> This is the more universal way of describing the port setup in an >>> OF-based way. There is also the DSA-specific (and old-style, before phylink) >>> way of describing the same thing, which relies on the non-OF-based >>> ds->slave_mii_bus, with bindings that look like this: >>> >>> ethernet-switch { >>> ethernet-ports { >>> port@0 { >>> reg = <0>; >>> }; >>> }; >>> }; >>> >>> But, I would say that the first variant of the binding is preferable, >>> since it is more universal. >>> >>> Not all switches that have an internal MDIO bus support the second >>> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't). >>> But, they support the same configuration through the first form. >> >> Understood. >> >>> >>> Furthermore, on the U-Boot mailing lists, I have been suggesting that >>> the DM_DSA driver for mv88e6xxx should not bother to support the second >>> version of the binding, since it is just more code to be added to handle >>> a case which can already be described with the more universal first binding. >> >> That makes sense. >> >>> >>>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC >>>> with a switch component. Since the switch component is not designed to be a >>>> standalone IC, the MDIO bus of the SoC could just as well be used without >>>> the need to design an MDIO controller specific to the switch component, to >>>> manage the PHYs. So I see this switch as just another case of a switch >>>> without an internal MDIO bus. >>> >>> Well, we need to clarify the semantics of an "internal" MDIO bus. >>> >>> I would say most discrete chips with DSA switches have this SoC-style >>> architecture, with separate address spaces for the switching IP, MDIO >>> bus, GPIO controller, IRQ controllers, temperature sensors etc (see >>> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is >>> controlled over SPIO instead of MMIO). The dt-bindings of most DSA >>> switches may or may not reflect that discrete chip organization. Those >>> drivers and dt-bindings could be reimagined so that DSA is not the >>> top-level driver. >>> >>> Yet, I would argue that it's wrong to say that because it isn't an OF >>> child of the switch, the MDIO bus of the VSC7514 is not internal in the >>> same way that the Realtek MDIO bus is internal. The switch ports are >>> connected to internal PHYs on this MDIO bus, and there aren't PHYs on >>> this MDIO bus that go to other MACs than the switch ports. So, the >>> VSC7514 MDIO bus could legally be called the internal MDIO bus of the >>> switch, even if there isn't a parent/child relationship between them. >> >> Good point, I had believed that the management interface of all of the PHYs >> being connected to the MDIO bus - which is not part of the switching IP >> address space - would be enough to classify the MDIO bus as non-internal. >> >> However, the architecture of separate address spaces for the switching IP >> and MDIO bus is used on any type of IC with the switching feature. >> Therefore, this characteristic cannot be used to distinguish whether an >> MDIO bus is of a switch. >> >> What we can refer to to classify an internal MDIO bus is by confirming the >> data interface of all PHYs on the MDIO bus is connected to the switch port >> MACs, as you have pointed out here. >> >> Because the architecture of separate address spaces for the switching IP >> and MDIO bus is used on any type of IC with the switching feature, it can >> differ by driver how the MDIO bus is defined on the dt-bindings. So we >> can't make universal bindings of an internal MDIO bus of a switch that >> apply to every switch. >> >> So, the correct approach is to define things under the switch-specific >> schema which is affine to the driver, as you have already pointed out. >> Which schemas to define what will of course differ. >> >>> >>> So, what I'm disagreeing with is your insistence to correlate your >>> problem with internal MDIO buses. The way in which the problem is >>> formulated dictates what problem gets solved, and the problem is not >>> correctly formulated here. It is purely about ds->slave_mii_bus and its >>> driver-defined OF presence/absence. It is a DSA-specific binding aspect >>> which not even all DSA switches inherit, let alone bindings outside DSA. >> >> Got it. >> >>> >>>>> For switches in the second category, it all depends on the way in which >>>>> the driver finds the node for of_mdiobus_register(). >>>> >>>> Ok, so some drivers require the mdio child node. Some require it and the >>>> compatible property with a certain string. >>>> >>>> MDIO controlled Realtek switches do not need the compatible property under >>>> the mdio child node. There're no compatible strings to make a distinction >>>> between the SMI and MDIO controlled switches so the best we can do is keep >>>> it the way it currently is. Define realtek,smi-mdio as a compatible string >>>> but keep the compatible property optional. I did state this on my reply to >>>> patch 3 but still received reviewed-bys regardless. >>> >>> Yes, because.... [1] >>> >>>>> Having identified all switches which make some sort of use of >>>>> ds->slave_mii_bus, the rule would sound like this: >>>>> >>>>> 1. If the schema is that of (need to replace this with compatible >>>>> strings, I'm too lazy for that): >>>>> >>>>> - ksz_switch_ops >>>>> - mv88e6060_switch_ops >>>>> - lan9303_switch_ops >>>>> - rtl8365mb_switch_ops_mdio >>>>> - b53_switch_ops >>>>> - vsc73xx_ds_ops >>>>> - mv88e6xxx >>>>> - qca8k >>>>> >>>>> and we have an "mdio" child, then phylink bindings are mandatory on user ports. >>>>> >>>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", >>>>> then phylink bindings are mandatory on user ports (I haven't checked, >>>>> but it might be that the "lantiq,xrx200-mdio" child is mandatory, and >>>>> in that case, this goes to category 4 below). >>>>> >>>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of >>>>> "realtek,smi-mdio", then phylink bindings are mandatory on user ports >>>>> (same comment about the child MDIO note maybe being mandatory). >>>>> >>>>> 4. If the switch didn't appear in the above set of rules, then phylink >>>>> bindings are unconditionally mandatory on user ports. >>>>> >>>>> We don't care at all what the drivers that don't use ds->slave_mii_bus >>>>> do with the "mdio" child node. It doesn't change the fact that their >>>>> user ports can't have missing phylink bindings. >>>> >>>> I partially agree. I say, for the switches without an internal MDIO bus, >>>> invalidate the mdio child node, and enforce the phylink bindings on the >>>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x, >>>> invalidate the mdio child node, and enforce the phylink bindings on the >>>> user ports if the mdios property is used. >>> >>> Why "if the mdios property is used" and not "always"? :-/ >>> >>> To say it again: because the sja1105 driver does not use ds->slave_mii_bus, >>> it can make no sense of dt-bindings on user ports which lack phylink properties. >>> So they are *always* needed. The "mdios" property changes nothing in that regard. >> >> Got it. >> >>> >>>> >>>> I'd like to add this before I conclude. The way I understand dt-bindings is >>>> that a binding does not have to translate to an action on the driver. >>>> Documenting bindings for the sole purpose of describing hardware is a valid >>>> case. >>> >>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are >>> otherwise the same, right? So why would they have different dt-bindings? >> >> Honestly, I'm wondering the answer to this as well. For some reason, when >> probing the SMI controlled Realtek switches, instead of just letting >> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() >> on realtek-smi.c: >> >> - priv->slave_mii_bus is allocated. >> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); >> - priv->slave_mii_bus->dev.of_node = mdio_np; >> - ds->slave_mii_bus = priv->slave_mii_bus; >> >>> >>>> For example, currently, the MT753X DSA subdriver won't, in any way, >>>> register the bus OF-based. Still, the mdio property for the switches which >>>> this driver controls can be documented because the internal mdio bus does >>>> exist on the hardware. >>> >>> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence, >>> then it loses its core functionality (that user ports can lack phylink >>> bindings). This is the entire essence of what this discussion should capture. >> >> Understood. >> >>> >>>> >>>> So I'd like to keep the mdio property valid for the switches which their >>>> drivers can only register non-OF-based ds->slave_mii_bus. >>>> >>>> In conclusion, what to do: >>>> >>>> - Define "the mdio property" and "the enforcement of phylink bindings for >>>> user ports if mdio property is used" on ethernet-switch.yaml. >>>> - Invalidate the mdio property on the switches without an internal MDIO >>>> bus. >>>> - Define "the enforcement of phylink bindings for user ports" on the >>>> switches without an internal MDIO bus. >>>> - Require "the mdio property" for the switches which their driver requires >>>> it to function. >>>> - Require "the mdio property" and "the compatible string of the mdio >>>> property" for the switches which their driver requires them to function. >>>> >>>> There's no 1:1 switch to switch compatible string relation, as seen on >>>> Realtek switches so I'll have to figure that out as I go. >>>> >>>> I'm open to your comments to this mail but the gap between discussion and >>>> end result has widened a lot on this patch series so I'd like to first >>>> offload this conversation by preparing v2 with what I said here and discuss >>>> further there. >>> >>> Honestly, from my side, a verbal comment in the dt-bindings document >>> would have been just fine, as long as it is truthful to the reality it >>> describes. >>> >>> You wanted to over-complicate things with an actual schema validation, >>> and then hooking onto things that are unrelated with the phenomenon that >>> needs to be captured (like the "mdio" child node, without explicit >>> regard to whether it is the ds->slave_mii_bus or not). >>> >>> It's not about internal MDIO buses in general, it's about whether those >>> internal MDIO buses are used in ds->slave_mii_bus, and their OF >>> presence/absence! That is absolutely driver-specific and I would only >>> expect a driver-specific way of enforcing it. I didn't say it's not >>> hard, and I didn't ask to enforce it, either. >> >> OK, I believe we're on the same page now, I will start working on properly >> enforcing this. > > I'm writing below as a mix of patch log and discussion. > > Phylink bindings are required for ports that are controlled by OF-based > buses. DSA, like any other driver utilising the Linux MDIO infrastructure, > can register a bus. If I understand correctly, non-OF-based registration of > OpenFirmware MDIO buses is a feature specific to DSA which certain DSA > subdrivers make use of. > > There's no way to distinguish which port is controlled by which driver's > MDIO bus on the bindings so we can't enforce phylink bindings for all user > ports as this would also enforce phylink bindings on user ports controlled > by a non-OF-based bus. > > But we can know when DSA won't create a non-OF-based bus. That leaves us > with only OF-based buses in which case we can enforce phylink bindings for > all user ports. So we need to check each DSA subdriver to see when all > buses will be OF-based. > > A DSA subdriver can either let the main DSA driver register the bus or it > can register a bus or buses itself. > > The attributes of a DSA subdriver that lets the DSA driver register the > bus: > - ds->ops->phy_read() and ds->ops->phy_write() are present. > - ds->slave_mii_bus is not populated by the DSA subdriver. > - The bus is registered non-OF-based or OF-based. Registered OF-based if > "mdio" child node is defined. > > How each DSA subdriver behaves is documented below. > > --- > > - mscc,vsc7514-switch.yaml > - mscc,vsc7514-switch > - mscc,vsc7512-switch > > drivers/net/ethernet/mscc/ocelot_vsc7514.c: > - Not a DSA subdriver. > - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used > to register a bus. > > drivers/net/dsa/ocelot/ocelot_ext.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used > to register a bus. > > What to do: > - For mscc,vsc7514-switch, enforce phylink bindings for ports. > - For mscc,vsc7512-switch, enforce phylink bindings for user ports. > > --- > > - renesas,rzn1-a5psw.yaml > - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw > > drivers/net/dsa/rzn1_a5psw.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers the bus OF-based only. Registers the bus when "mdio" child node > is defined. > - mdio = of_get_child_by_name(dev->of_node, "mdio"); > if (of_device_is_available(mdio)) > ret = a5psw_probe_mdio(a5psw, mdio); > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports. > > --- > > - realtek.yaml > - realtek,rtl8365mb > - realtek,rtl8366rb > > drivers/net/dsa/realtek/realtek-smi.c: > - The DSA subdriver won't let the DSA driver register the bus. Registers > the bus OF-based only. Registering the bus is mandatory. Registers the > bus when compatible string "realtek,smi-mdio" on a child node is defined. > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); > if (!mdio_np) > return -ENODEV; > ds->slave_mii_bus = priv->slave_mii_bus; > > drivers/net/dsa/realtek/realtek-mdio.c: > - The DSA subdriver lets the DSA driver register the bus. > > What to do: > - Document "mdio". > - Require "mdio". (Can't do because it's not required for MDIO controlled > switches that share the compatible string with SMI controlled switches. > This is why I would like Luiz to unify the bus registeration process.) > - Document compatible string "realtek,smi-mdio" on "mdio" child node. > - Require compatible. (Can't do because the same as above.) > - Enforce phylink bindings for user ports. (Can't do because the same as > above.) > - Enforce phylink bindings for user ports if "mdio" is defined. I rediscovered that the reg property can be used to distinguish the SMI and MDIO controlled Realtek switches. So I can indeed write a proper schema. I'd still like the bus registration to be unified to eliminate unnecessary complexity. > > --- > > - qca8k.yaml > - qca,qca8327 > - qca,qca8328 > - qca,qca8334 > - qca,qca8337 > > drivers/net/dsa/qca/qca8k-8xxx.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" > child node is defined. > - mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - nxp,sja1105.yaml > - nxp,sja1105e > - nxp,sja1105t > - nxp,sja1105p > - nxp,sja1105q > - nxp,sja1105r > - nxp,sja1105s > - nxp,sja1110a > - nxp,sja1110b > - nxp,sja1110c > - nxp,sja1110d > > drivers/net/dsa/sja1105/sja1105_mdio.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers multiple buses OF-based only. Registers the buses when "mdios" > child node and "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio" > compatible strings per child node under "mdios" is defined. > - mdio_node = of_get_child_by_name(switch_node, "mdios"); > if (!mdio_node) > return 0; > - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio"); > if (!np) > return 0; > - np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio"); > if (!np) > return 0; > > What to do: > - Document "mdios". > - Document child node pattern property under "mdios". > - Document "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio" > compatible strings. > - Enforce phylink bindings for user ports. > > --- > > - mscc,ocelot.yaml > - mscc,vsc9953-switch > - pci1957,eef0 > > drivers/net/dsa/ocelot/seville_vsc9953.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - MDIO_MSCC_MIIM, a driver utilising the Linux MDIO infrastructure is used > to register a bus. > > drivers/net/dsa/ocelot/felix_vsc9959.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - FSL_ENETC_MDIO, a driver utilising the Linux MDIO infrastructure is used > to register a bus. > > What to do: > - Enforce phylink bindings for user ports. > > --- > > - microchip,lan937x.yaml > - microchip,lan9370 > - microchip,lan9371 > - microchip,lan9372 > - microchip,lan9373 > - microchip,lan9374 > - microchip,ksz.yaml > - microchip,ksz8765 > - microchip,ksz8794 > - microchip,ksz8795 > - microchip,ksz8863 > - microchip,ksz8873 > - microchip,ksz9477 > - microchip,ksz9897 > - microchip,ksz9896 > - microchip,ksz9567 > - microchip,ksz8565 > - microchip,ksz9893 > - microchip,ksz9563 > - microchip,ksz8563 > > drivers/net/dsa/microchip/ksz_common.c: > - The DSA subdriver won't let the DSA driver register the bus when "mdio" > child node is defined. Registers the bus OF-based only. Registers the bus > when "mdio" child node is defined. > - mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio"); > if (!mdio_np) > return 0; > ds->slave_mii_bus = bus; > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - hirschmann,hellcreek.yaml > - hirschmann,hellcreek-de1soc-r1 > > drivers/net/dsa/hirschmann/hellcreek.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > > What to do: > - Enforce phylink bindings for user ports. > > --- > > - mediatek,mt7530.yaml > - mediatek,mt7530 > - mediatek,mt7531 > - mediatek,mt7621 > - mediatek,mt7988-switch > > drivers/net/dsa/mt7530.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" > child node is defined. (This is after the patch for the MT7530 DSA > subdriver is applied.) > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - brcm,sf2.yaml > - brcm,bcm4908-switch > - brcm,bcm7278-switch-v4.0 > - brcm,bcm7278-switch-v4.8 > - brcm,bcm7445-switch-v4.0 > > drivers/net/dsa/bcm_sf2.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Requires MDIO_BCM_UNIMAC, a driver utilising the Linux MDIO > infrastructure to register a bus. > - dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio"); > priv->master_mii_bus = of_mdio_find_bus(dn); > if (!priv->master_mii_bus) { > of_node_put(dn); > return -EPROBE_DEFER; > } > > What to do: > - Enforce phylink bindings for user ports. > > --- > > - brcm,b53.yaml > - brcm,bcm5325 > - brcm,bcm53115 > - brcm,bcm53125 > - brcm,bcm53128 > - brcm,bcm53134 > - brcm,bcm5365 > - brcm,bcm5395 > - brcm,bcm5389 > - brcm,bcm5397 > - brcm,bcm5398 > - brcm,bcm11360-srab, brcm,cygnus-srab > - brcm,bcm53010-srab, brcm,bcm5301x-srab > - brcm,bcm53011-srab, brcm,bcm5301x-srab > - brcm,bcm53012-srab, brcm,bcm5301x-srab > - brcm,bcm53018-srab, brcm,bcm5301x-srab > - brcm,bcm53019-srab, brcm,bcm5301x-srab > - brcm,bcm11404-srab, brcm,omega-srab > - brcm,bcm11407-srab, brcm,omega-srab > - brcm,bcm11409-srab, brcm,omega-srab > - brcm,bcm58310-srab, brcm,omega-srab > - brcm,bcm58311-srab, brcm,omega-srab > - brcm,bcm58313-srab, brcm,omega-srab > - brcm,bcm58522-srab, brcm,nsp-srab > - brcm,bcm58523-srab, brcm,nsp-srab > - brcm,bcm58525-srab, brcm,nsp-srab > - brcm,bcm58622-srab, brcm,nsp-srab > - brcm,bcm58623-srab, brcm,nsp-srab > - brcm,bcm58625-srab, brcm,nsp-srab > - brcm,bcm88312-srab, brcm,nsp-srab > - brcm,bcm3384-switch, brcm,bcm63xx-switch > - brcm,bcm6318-switch, brcm,bcm63xx-switch > - brcm,bcm6328-switch, brcm,bcm63xx-switch > - brcm,bcm6362-switch, brcm,bcm63xx-switch > - brcm,bcm6368-switch, brcm,bcm63xx-switch > - brcm,bcm63268-switch, brcm,bcm63xx-switch > > drivers/net/dsa/b53/b53_common.c: > - The DSA subdriver lets the DSA driver register the bus. > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - arrow,xrs700x.yaml > - arrow,xrs7003e > - arrow,xrs7003f > - arrow,xrs7004e > - arrow,xrs7004f > > drivers/net/dsa/xrs700x/xrs700x.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > > What to do: > - Enforce phylink bindings for user ports. > > --- > > Text documents. I will handle these when I replace them with json-schema > documents. > > - ar9331.txt > - qca,ar9331-switch > > drivers/net/dsa/qca/ar9331.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers the bus OF-based only. Registering the bus is mandatory. > Registers the bus when "mdio" child node is defined. > - mnp = of_get_child_by_name(np, "mdio"); > if (!mnp) > return -ENODEV; > > What to do: > - Document "mdio". > - Require "mdio". > - Enforce phylink bindings for user ports. > > --- > > - lan9303.txt > - smsc,lan9303-i2c > - smsc,lan9303-mdio > > drivers/net/dsa/lan9303-core.c: > - The DSA subdriver lets the DSA driver register the bus. > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - lantiq-gswip.txt > - lantiq,xrx200-gswip > - lantiq,xrx300-gswip > - lantiq,xrx330-gswip > > drivers/net/dsa/lantiq_gswip.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers the bus OF-based only. Registers the bus when compatible string > "lantiq,xrx200-mdio" on a child node is defined. > - mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio"); > if (mdio_np) > err = gswip_mdio(priv, mdio_np); > > What to do: > - Document "mdio". > - Document compatible string "realtek,smi-mdio" on "mdio" child node. This should be "lantiq,xrx200-mdio" instead. Arınç > - Require compatible. > - Enforce phylink bindings for user ports. > > --- > > - marvell.txt > - marvell,mv88e6085 > - marvell,mv88e6190 > - marvell,mv88e6250 > > drivers/net/dsa/mv88e6xxx/chip.c: > - The DSA subdriver won't let the DSA driver register the bus. > - ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip); > - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" > child node is defined. > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - vitesse,vsc73xx.txt > - vitesse,vsc7385 > - vitesse,vsc7388 > - vitesse,vsc7395 > - vitesse,vsc7398 > > drivers/net/dsa/vitesse-vsc73xx-core.c: > - The DSA subdriver lets the DSA driver register the bus. > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > Arınç
On 9.09.2023 11:53, Arınç ÜNAL wrote: > On 4.09.2023 14:33, Arınç ÜNAL wrote: >> Hey Vladimir, >> >> On 27.08.2023 15:12, Vladimir Oltean wrote: >>> Hi Arınç, >>> >>> I am on vacation and I will just reply with some clarification aspects, >>> without having done any further research on the topic since my last reply. >>> >>> On Sun, Aug 27, 2023 at 11:33:16AM +0300, Arınç ÜNAL wrote: >>>> Before I continue commenting, I'd like to state my understanding so we can >>>> make sure we're on the same page. If a driver doesn't use >>>> ds->slave_mii_bus, the switch it controls must not have any internal MDIO >>>> buses. Otherwise the PHYs on these buses couldn't function, and an improper >>>> driver like this would not be on the official Linux source code. >>> >>> A DSA switch port, like any OF-based ethernet-controller which uses >>> phylink, will use one of the phy-handle, fixed-link or managed properties >>> to describe the interface connecting the MAC/MAC-side PCS to the PHY. >>> >>> At its core, ds->slave_mii_bus is nothing more than a mechanism to make >>> sense of device trees where the above 3 phylink properties are not present. >>> >>> It is completely false to say that if a driver doesn't have ds->slave_mii_bus, >>> it must not have an internal MDIO bus. Because you could still describe >>> that internal MDIO bus like below, without making any use of the sole property >>> that makes ds->slave_mii_bus useful from a dt-bindings perspective. >>> >>> ethernet-switch { >>> ethernet-ports { >>> port@0 { >>> reg = <0>; >>> phy-handle = <&port0_phy>; >>> phy-mode = "internal"; >>> }; >>> }; >>> >>> mdio { >>> port0_phy: ethernet-phy@0 { >>> reg = <0>; >>> }; >>> }; >>> }; >>> >>> This is the more universal way of describing the port setup in an >>> OF-based way. There is also the DSA-specific (and old-style, before phylink) >>> way of describing the same thing, which relies on the non-OF-based >>> ds->slave_mii_bus, with bindings that look like this: >>> >>> ethernet-switch { >>> ethernet-ports { >>> port@0 { >>> reg = <0>; >>> }; >>> }; >>> }; >>> >>> But, I would say that the first variant of the binding is preferable, >>> since it is more universal. >>> >>> Not all switches that have an internal MDIO bus support the second >>> variant of the dt-binding (the ones that don't have ds->slave_mii_bus don't). >>> But, they support the same configuration through the first form. >> >> Understood. >> >>> >>> Furthermore, on the U-Boot mailing lists, I have been suggesting that >>> the DM_DSA driver for mv88e6xxx should not bother to support the second >>> version of the binding, since it is just more code to be added to handle >>> a case which can already be described with the more universal first binding. >> >> That makes sense. >> >>> >>>> I've checked mscc,vsc7514-switch. What I see is, the architecture is an SoC >>>> with a switch component. Since the switch component is not designed to be a >>>> standalone IC, the MDIO bus of the SoC could just as well be used without >>>> the need to design an MDIO controller specific to the switch component, to >>>> manage the PHYs. So I see this switch as just another case of a switch >>>> without an internal MDIO bus. >>> >>> Well, we need to clarify the semantics of an "internal" MDIO bus. >>> >>> I would say most discrete chips with DSA switches have this SoC-style >>> architecture, with separate address spaces for the switching IP, MDIO >>> bus, GPIO controller, IRQ controllers, temperature sensors etc (see >>> "mscc,vsc7512-switch" which is like "mscc,vsc7514-switch", but it is >>> controlled over SPIO instead of MMIO). The dt-bindings of most DSA >>> switches may or may not reflect that discrete chip organization. Those >>> drivers and dt-bindings could be reimagined so that DSA is not the >>> top-level driver. >>> >>> Yet, I would argue that it's wrong to say that because it isn't an OF >>> child of the switch, the MDIO bus of the VSC7514 is not internal in the >>> same way that the Realtek MDIO bus is internal. The switch ports are >>> connected to internal PHYs on this MDIO bus, and there aren't PHYs on >>> this MDIO bus that go to other MACs than the switch ports. So, the >>> VSC7514 MDIO bus could legally be called the internal MDIO bus of the >>> switch, even if there isn't a parent/child relationship between them. >> >> Good point, I had believed that the management interface of all of the PHYs >> being connected to the MDIO bus - which is not part of the switching IP >> address space - would be enough to classify the MDIO bus as non-internal. >> >> However, the architecture of separate address spaces for the switching IP >> and MDIO bus is used on any type of IC with the switching feature. >> Therefore, this characteristic cannot be used to distinguish whether an >> MDIO bus is of a switch. >> >> What we can refer to to classify an internal MDIO bus is by confirming the >> data interface of all PHYs on the MDIO bus is connected to the switch port >> MACs, as you have pointed out here. >> >> Because the architecture of separate address spaces for the switching IP >> and MDIO bus is used on any type of IC with the switching feature, it can >> differ by driver how the MDIO bus is defined on the dt-bindings. So we >> can't make universal bindings of an internal MDIO bus of a switch that >> apply to every switch. >> >> So, the correct approach is to define things under the switch-specific >> schema which is affine to the driver, as you have already pointed out. >> Which schemas to define what will of course differ. >> >>> >>> So, what I'm disagreeing with is your insistence to correlate your >>> problem with internal MDIO buses. The way in which the problem is >>> formulated dictates what problem gets solved, and the problem is not >>> correctly formulated here. It is purely about ds->slave_mii_bus and its >>> driver-defined OF presence/absence. It is a DSA-specific binding aspect >>> which not even all DSA switches inherit, let alone bindings outside DSA. >> >> Got it. >> >>> >>>>> For switches in the second category, it all depends on the way in which >>>>> the driver finds the node for of_mdiobus_register(). >>>> >>>> Ok, so some drivers require the mdio child node. Some require it and the >>>> compatible property with a certain string. >>>> >>>> MDIO controlled Realtek switches do not need the compatible property under >>>> the mdio child node. There're no compatible strings to make a distinction >>>> between the SMI and MDIO controlled switches so the best we can do is keep >>>> it the way it currently is. Define realtek,smi-mdio as a compatible string >>>> but keep the compatible property optional. I did state this on my reply to >>>> patch 3 but still received reviewed-bys regardless. >>> >>> Yes, because.... [1] >>> >>>>> Having identified all switches which make some sort of use of >>>>> ds->slave_mii_bus, the rule would sound like this: >>>>> >>>>> 1. If the schema is that of (need to replace this with compatible >>>>> strings, I'm too lazy for that): >>>>> >>>>> - ksz_switch_ops >>>>> - mv88e6060_switch_ops >>>>> - lan9303_switch_ops >>>>> - rtl8365mb_switch_ops_mdio >>>>> - b53_switch_ops >>>>> - vsc73xx_ds_ops >>>>> - mv88e6xxx >>>>> - qca8k >>>>> >>>>> and we have an "mdio" child, then phylink bindings are mandatory on user ports. >>>>> >>>>> 2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio", >>>>> then phylink bindings are mandatory on user ports (I haven't checked, >>>>> but it might be that the "lantiq,xrx200-mdio" child is mandatory, and >>>>> in that case, this goes to category 4 below). >>>>> >>>>> 3. If the schema is that of realtek_smi_setup_mdio and we have a child node of >>>>> "realtek,smi-mdio", then phylink bindings are mandatory on user ports >>>>> (same comment about the child MDIO note maybe being mandatory). >>>>> >>>>> 4. If the switch didn't appear in the above set of rules, then phylink >>>>> bindings are unconditionally mandatory on user ports. >>>>> >>>>> We don't care at all what the drivers that don't use ds->slave_mii_bus >>>>> do with the "mdio" child node. It doesn't change the fact that their >>>>> user ports can't have missing phylink bindings. >>>> >>>> I partially agree. I say, for the switches without an internal MDIO bus, >>>> invalidate the mdio child node, and enforce the phylink bindings on the >>>> user ports. Such as mscc,vsc7514-switch and nxp,sja1105x. For nxp,sja1110x, >>>> invalidate the mdio child node, and enforce the phylink bindings on the >>>> user ports if the mdios property is used. >>> >>> Why "if the mdios property is used" and not "always"? :-/ >>> >>> To say it again: because the sja1105 driver does not use ds->slave_mii_bus, >>> it can make no sense of dt-bindings on user ports which lack phylink properties. >>> So they are *always* needed. The "mdios" property changes nothing in that regard. >> >> Got it. >> >>> >>>> >>>> I'd like to add this before I conclude. The way I understand dt-bindings is >>>> that a binding does not have to translate to an action on the driver. >>>> Documenting bindings for the sole purpose of describing hardware is a valid >>>> case. >>> >>> [1] ...this. The SMI-controlled and MDIO-controlled Realtek switches are >>> otherwise the same, right? So why would they have different dt-bindings? >> >> Honestly, I'm wondering the answer to this as well. For some reason, when >> probing the SMI controlled Realtek switches, instead of just letting >> dsa_switch_setup() populate ds->slave_mii_bus, on realtek_smi_setup_mdio() >> on realtek-smi.c: >> >> - priv->slave_mii_bus is allocated. >> - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio"); >> - priv->slave_mii_bus->dev.of_node = mdio_np; >> - ds->slave_mii_bus = priv->slave_mii_bus; >> >>> >>>> For example, currently, the MT753X DSA subdriver won't, in any way, >>>> register the bus OF-based. Still, the mdio property for the switches which >>>> this driver controls can be documented because the internal mdio bus does >>>> exist on the hardware. >>> >>> It can, but the whole point is: if ds->slave_mii_bus gains an OF presence, >>> then it loses its core functionality (that user ports can lack phylink >>> bindings). This is the entire essence of what this discussion should capture. >> >> Understood. >> >>> >>>> >>>> So I'd like to keep the mdio property valid for the switches which their >>>> drivers can only register non-OF-based ds->slave_mii_bus. >>>> >>>> In conclusion, what to do: >>>> >>>> - Define "the mdio property" and "the enforcement of phylink bindings for >>>> user ports if mdio property is used" on ethernet-switch.yaml. >>>> - Invalidate the mdio property on the switches without an internal MDIO >>>> bus. >>>> - Define "the enforcement of phylink bindings for user ports" on the >>>> switches without an internal MDIO bus. >>>> - Require "the mdio property" for the switches which their driver requires >>>> it to function. >>>> - Require "the mdio property" and "the compatible string of the mdio >>>> property" for the switches which their driver requires them to function. >>>> >>>> There's no 1:1 switch to switch compatible string relation, as seen on >>>> Realtek switches so I'll have to figure that out as I go. >>>> >>>> I'm open to your comments to this mail but the gap between discussion and >>>> end result has widened a lot on this patch series so I'd like to first >>>> offload this conversation by preparing v2 with what I said here and discuss >>>> further there. >>> >>> Honestly, from my side, a verbal comment in the dt-bindings document >>> would have been just fine, as long as it is truthful to the reality it >>> describes. >>> >>> You wanted to over-complicate things with an actual schema validation, >>> and then hooking onto things that are unrelated with the phenomenon that >>> needs to be captured (like the "mdio" child node, without explicit >>> regard to whether it is the ds->slave_mii_bus or not). >>> >>> It's not about internal MDIO buses in general, it's about whether those >>> internal MDIO buses are used in ds->slave_mii_bus, and their OF >>> presence/absence! That is absolutely driver-specific and I would only >>> expect a driver-specific way of enforcing it. I didn't say it's not >>> hard, and I didn't ask to enforce it, either. >> >> OK, I believe we're on the same page now, I will start working on properly >> enforcing this. > > I'm writing below as a mix of patch log and discussion. > > Phylink bindings are required for ports that are controlled by OF-based > buses. DSA, like any other driver utilising the Linux MDIO infrastructure, > can register a bus. If I understand correctly, non-OF-based registration of > OpenFirmware MDIO buses is a feature specific to DSA which certain DSA > subdrivers make use of. > > There's no way to distinguish which port is controlled by which driver's > MDIO bus on the bindings so we can't enforce phylink bindings for all user > ports as this would also enforce phylink bindings on user ports controlled > by a non-OF-based bus. > > But we can know when DSA won't create a non-OF-based bus. That leaves us > with only OF-based buses in which case we can enforce phylink bindings for > all user ports. So we need to check each DSA subdriver to see when all > buses will be OF-based. We also need to decide the phylink bindings for user ports. Phylink bindings for CPU and DSA ports: allOf: - required: - phy-mode - oneOf: - required: - fixed-link - required: - phy-handle - required: - managed On one of the mscc,ocelot.yaml examples, "phy-handle" and "managed" are defined on the same user port. Assuming the example is correct, we must allow more than 1 of these properties to be used at the same time for user ports. We need to at least allow "phy-handle" and "managed" to be used at the same time. Does "managed" also depend on "phy-handle"? For example: oneOf: - required: - fixed-link - anyOf: - required: - phy-handle - required: - managed dependencies: managed: [ phy-handle ] Arınç
Please trim the text when replying. > I'm writing below as a mix of patch log and discussion. > > Phylink bindings are required for ports that are controlled by OF-based > buses. DSA, like any other driver utilising the Linux MDIO infrastructure, > can register a bus. If I understand correctly, non-OF-based registration of > OpenFirmware MDIO buses is a feature specific to DSA which certain DSA > subdrivers make use of. This is not really DSA specific. Any MAC driver, or MDIO driver, can call mdiobus_regsiter(), or of_mdiobus_register() and pass a NULL pointer if there is no OF node available. It then requires that the MAC driver uses an function like phy_find_first(), or knows via other means what address the PHY uses on the bus. For DSA, that other means is assuming a 1:1 mapping between port number and bus address. Andrew
On Sat, Sep 09, 2023 at 11:53:50AM +0300, Arınç ÜNAL wrote: > What to do: > - For mscc,vsc7514-switch, enforce phylink bindings for ports. > - For mscc,vsc7512-switch, enforce phylink bindings for user ports. you can also look at dsa_switches_apply_workarounds[], and if the switch isn't there, then you can replace "user ports" with "ports" here and everywhere. > - renesas,rzn1-a5psw.yaml > - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw > > What to do: > - Document "mdio". Not clear here and for all the schemas quoted below.. is "mdio" not documented already? > - realtek.yaml > - realtek,rtl8365mb > - realtek,rtl8366rb > > drivers/net/dsa/realtek/realtek-mdio.c: > - The DSA subdriver lets the DSA driver register the bus. > > What to do: > - Document "mdio". > - Require "mdio". (Can't do because it's not required for MDIO controlled > switches that share the compatible string with SMI controlled switches. > This is why I would like Luiz to unify the bus registeration process.) > - Document compatible string "realtek,smi-mdio" on "mdio" child node. > - Require compatible. (Can't do because the same as above.) > - Enforce phylink bindings for user ports. (Can't do because the same as > above.) > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - qca8k.yaml > - qca,qca8327 > - qca,qca8328 > - qca,qca8334 > - qca,qca8337 > > drivers/net/dsa/qca/qca8k-8xxx.c: > - The DSA subdriver won't let the DSA driver register the bus. > - No ds->ops->phy_read() or ds->ops->phy_write(). > - Registers the bus non-OF-based or OF-based. Registers OF-based if "mdio" > child node is defined. > - mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); > > What to do: > - Document "mdio". > - Enforce phylink bindings for user ports if "mdio" is defined. > > --- > > - nxp,sja1105.yaml > - nxp,sja1105e > - nxp,sja1105t > - nxp,sja1105p > - nxp,sja1105q > - nxp,sja1105r > - nxp,sja1105s > - nxp,sja1110a > - nxp,sja1110b > - nxp,sja1110c > - nxp,sja1110d > > What to do: > - Document "mdios". > - Document child node pattern property under "mdios". > - Document "nxp,sja1110-base-tx-mdio" and "nxp,sja1110-base-t1-mdio" > compatible strings. > --- > > - microchip,lan937x.yaml > - microchip,lan9370 > - microchip,lan9371 > - microchip,lan9372 > - microchip,lan9373 > - microchip,lan9374 > - microchip,ksz.yaml > - microchip,ksz8765 > - microchip,ksz8794 > - microchip,ksz8795 > - microchip,ksz8863 > - microchip,ksz8873 > - microchip,ksz9477 > - microchip,ksz9897 > - microchip,ksz9896 > - microchip,ksz9567 > - microchip,ksz8565 > - microchip,ksz9893 > - microchip,ksz9563 > - microchip,ksz8563 > > What to do: > - Document "mdio".
On 10.09.2023 00:16, Andrew Lunn wrote: > Please trim the text when replying. > > >> I'm writing below as a mix of patch log and discussion. >> >> Phylink bindings are required for ports that are controlled by OF-based >> buses. DSA, like any other driver utilising the Linux MDIO infrastructure, >> can register a bus. If I understand correctly, non-OF-based registration of >> OpenFirmware MDIO buses is a feature specific to DSA which certain DSA >> subdrivers make use of. > > This is not really DSA specific. Any MAC driver, or MDIO driver, can > call mdiobus_regsiter(), or of_mdiobus_register() and pass a NULL > pointer if there is no OF node available. It then requires that the > MAC driver uses an function like phy_find_first(), or knows via other > means what address the PHY uses on the bus. For DSA, that other means > is assuming a 1:1 mapping between port number and bus address. Understood. At least a phy-handle on the DSA user port for each PHY controlled by non-DSA drivers is always needed correct? Otherwise DSA wouldn't know which PHY to map the DSA switch port? And that means DSA requires OF-based buses to map the ports controlled by non-DSA driver buses to PHYs? I'm trying to understand if phylink bindings for DSA user ports that are controlled by non-DSA driver buses are always necessary. Would this also apply to MAC drivers that control switches? Arınç
On 12.09.2023 01:51, Vladimir Oltean wrote: > On Sat, Sep 09, 2023 at 11:53:50AM +0300, Arınç ÜNAL wrote: >> What to do: >> - For mscc,vsc7514-switch, enforce phylink bindings for ports. >> - For mscc,vsc7512-switch, enforce phylink bindings for user ports. > > you can also look at dsa_switches_apply_workarounds[], and if the switch > isn't there, then you can replace "user ports" with "ports" here and > everywhere. The phylink bindings for user ports I ended up making by looking up the existing devicetrees are different than the phylink bindings for the shared (CPU and DSA) ports currently enforced on all switches. My phylink bindings for user ports: allOf: - anyOf: - required: [ fixed-link ] - required: [ phy-handle ] - required: [ managed ] - if: required: [ fixed-link ] then: not: required: [ managed ] The phylink bindings for shared ports enforced on all switches on dsa-port.yaml: allOf: - required: - phy-mode - oneOf: - required: - fixed-link - required: - phy-handle - required: - managed Here's what I understand: - For switches in dsa_switches_apply_workarounds[] - Enforce the latter for shared ports. - Enforce the former for user ports. - For switches not in dsa_switches_apply_workarounds[] - Enforce the former for all ports. > >> - renesas,rzn1-a5psw.yaml >> - renesas,r9a06g032-a5psw, renesas,rzn1-a5psw >> >> What to do: >> - Document "mdio". > > Not clear here and for all the schemas quoted below.. is "mdio" not documented already? They are, or rather I didn't care while constructing this text. I will mention "mdio" is already documented per schema on the patch log. Arınç
On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote: > On 12.09.2023 01:51, Vladimir Oltean wrote: > > On Sat, Sep 09, 2023 at 11:53:50AM +0300, Arınç ÜNAL wrote: > > > What to do: > > > - For mscc,vsc7514-switch, enforce phylink bindings for ports. > > > - For mscc,vsc7512-switch, enforce phylink bindings for user ports. > > > > you can also look at dsa_switches_apply_workarounds[], and if the switch > > isn't there, then you can replace "user ports" with "ports" here and > > everywhere. > > The phylink bindings for user ports I ended up making by looking up the > existing devicetrees are different than the phylink bindings for the shared > (CPU and DSA) ports currently enforced on all switches. > > My phylink bindings for user ports: > > allOf: > - anyOf: > - required: [ fixed-link ] > - required: [ phy-handle ] > - required: [ managed ] > > - if: > required: [ fixed-link ] > then: > not: > required: [ managed ] Right, it should have been anyOf and not oneOf.. my mistake. It is a bug which should be fixed. It's the same phylink that gets used in both cases, user ports and shared ports :) > > The phylink bindings for shared ports enforced on all switches on > dsa-port.yaml: > > allOf: > - required: > - phy-mode > - oneOf: > - required: > - fixed-link > - required: > - phy-handle > - required: > - managed > > Here's what I understand: > > - For switches in dsa_switches_apply_workarounds[] > - Enforce the latter for shared ports. > - Enforce the former for user ports. > > - For switches not in dsa_switches_apply_workarounds[] > - Enforce the former for all ports. No, no. We enforce the dt-schema regardless of switch presence in dsa_switches_apply_workarounds[], to encourage users to fix device trees (those who run schema validation). The kernel workaround consists in doing something (skipping phylink) for the device trees where the schema warns on shared ports. But there should be a single sub-schema for validating phylink bindings, whatever port kind it is.
> > The marvell switch can have up to 2 MDIO busses. If i remember > > correctly, there is also one switch which has one MDIO bus per port. > > I'm writing the json-schema for Marvell switches. I've checked a few > devicetree source files on Linus's Linux tree, I only see two buses used at > the most. Sorry, i was ambiguous. Its not a Marvell switch which can have one MDIO bus per port. I don't remember which switch it is, and it might be a pure switchdev switch, not a DSA switch. > The internal bus and another bus with > marvell,mv88e6xxx-mdio-external. I've never seen a devicetree with > marvell,mv88e6250 though. Could the switch that has one MDIO bus per port > be this one? Also, is every bus of this switch a > marvell,mv88e6xxx-mdio-external bus or, one internal bus and the remaining > are marvell mv88e6xxx-mdio-external buses? Only the 6390 family has two busses. It has an internal MDIO bus with the same register API as all the other switches. However, unlike the other families, it is not exposed on pins. And the 6390 has a second MDIO bus using a slight variant of the registers, which is connected to the outside world via pins. This second bus then has a compatible to separate it from the normal MDIO bus. Andrew
On 12.09.2023 22:34, Vladimir Oltean wrote: > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote: >> The phylink bindings for user ports I ended up making by looking up the >> existing devicetrees are different than the phylink bindings for the shared >> (CPU and DSA) ports currently enforced on all switches. >> >> My phylink bindings for user ports: >> >> allOf: >> - anyOf: >> - required: [ fixed-link ] >> - required: [ phy-handle ] >> - required: [ managed ] >> >> - if: >> required: [ fixed-link ] >> then: >> not: >> required: [ managed ] > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug > which should be fixed. It's the same phylink that gets used in both cases, > user ports and shared ports :) One more thing, I don't recall phy-mode being required to be defined for user ports as it will default to GMII. I don't believe this is the same case for shared ports so phy-mode is required only for them? > >> >> The phylink bindings for shared ports enforced on all switches on >> dsa-port.yaml: >> >> allOf: >> - required: >> - phy-mode >> - oneOf: >> - required: >> - fixed-link >> - required: >> - phy-handle >> - required: >> - managed >> >> Here's what I understand: >> >> - For switches in dsa_switches_apply_workarounds[] >> - Enforce the latter for shared ports. >> - Enforce the former for user ports. >> >> - For switches not in dsa_switches_apply_workarounds[] >> - Enforce the former for all ports. > > No, no. We enforce the dt-schema regardless of switch presence in > dsa_switches_apply_workarounds[], to encourage users to fix device trees > (those who run schema validation). The kernel workaround consists in > doing something (skipping phylink) for the device trees where the schema > warns on shared ports. But there should be a single sub-schema for > validating phylink bindings, whatever port kind it is. Hmm, like writing phylink.yaml and then referring to it under the port pattern node? This could prevent a lot of repetition. Arınç
On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote: > On 12.09.2023 22:34, Vladimir Oltean wrote: > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote: > > > The phylink bindings for user ports I ended up making by looking up the > > > existing devicetrees are different than the phylink bindings for the shared > > > (CPU and DSA) ports currently enforced on all switches. > > > > > > My phylink bindings for user ports: > > > > > > allOf: > > > - anyOf: > > > - required: [ fixed-link ] > > > - required: [ phy-handle ] > > > - required: [ managed ] > > > > > > - if: > > > required: [ fixed-link ] > > > then: > > > not: > > > required: [ managed ] > > > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug > > which should be fixed. It's the same phylink that gets used in both cases, > > user ports and shared ports :) > > One more thing, I don't recall phy-mode being required to be defined for > user ports as it will default to GMII. I don't believe this is the same > case for shared ports so phy-mode is required only for them? phy-mode is not strictly required, but I think there is a strong preference to set it. IIRC, when looking at the DSA device trees, there was no case where phy-mode would be absent on CPU/DSA ports if the other link properties were also present, so we required it too. There were no complaints in 1 year since dsa_shared_port_validate_of() is there. The requirement can be relaxed to just a warning and no error in the kernel, and the removal of "required" in the schema, if it helps making it common with user ports. I think that the fallback to PHY_INTERFACE_MODE_GMII applies only if there is a phy_device (phy-handle). But otherwise, I don't remember if the PHY_INTERFACE_MODE_NA passed to phylink_create() will persist at runtime, or cause an error somewhere. > > > The phylink bindings for shared ports enforced on all switches on > > > dsa-port.yaml: > > > > > > allOf: > > > - required: > > > - phy-mode > > > - oneOf: > > > - required: > > > - fixed-link > > > - required: > > > - phy-handle > > > - required: > > > - managed > > > > > > Here's what I understand: > > > > > > - For switches in dsa_switches_apply_workarounds[] > > > - Enforce the latter for shared ports. > > > - Enforce the former for user ports. > > > > > > - For switches not in dsa_switches_apply_workarounds[] > > > - Enforce the former for all ports. > > > > No, no. We enforce the dt-schema regardless of switch presence in > > dsa_switches_apply_workarounds[], to encourage users to fix device trees > > (those who run schema validation). The kernel workaround consists in > > doing something (skipping phylink) for the device trees where the schema > > warns on shared ports. But there should be a single sub-schema for > > validating phylink bindings, whatever port kind it is. > > Hmm, like writing phylink.yaml and then referring to it under the port > pattern node? This could prevent a lot of repetition. > > Arınç Yes, that would sound good.
On 13.09.2023 10:42, Vladimir Oltean wrote: > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote: >> On 12.09.2023 22:34, Vladimir Oltean wrote: >>> Right, it should have been anyOf and not oneOf.. my mistake. It is a bug >>> which should be fixed. It's the same phylink that gets used in both cases, >>> user ports and shared ports :) >> >> One more thing, I don't recall phy-mode being required to be defined for >> user ports as it will default to GMII. I don't believe this is the same >> case for shared ports so phy-mode is required only for them? > > phy-mode is not strictly required, but I think there is a strong > preference to set it. IIRC, when looking at the DSA device trees, there > was no case where phy-mode would be absent on CPU/DSA ports if the other > link properties were also present, so we required it too. There were no > complaints in 1 year since dsa_shared_port_validate_of() is there. The > requirement can be relaxed to just a warning and no error in the kernel, > and the removal of "required" in the schema, if it helps making it > common with user ports. I'd say no need as it doesn't make it complicated that much. See below. > > I think that the fallback to PHY_INTERFACE_MODE_GMII applies only if > there is a phy_device (phy-handle). But otherwise, I don't remember if > the PHY_INTERFACE_MODE_NA passed to phylink_create() will persist at > runtime, or cause an error somewhere. > >>>> The phylink bindings for shared ports enforced on all switches on >>>> dsa-port.yaml: >>>> >>>> allOf: >>>> - required: >>>> - phy-mode >>>> - oneOf: >>>> - required: >>>> - fixed-link >>>> - required: >>>> - phy-handle >>>> - required: >>>> - managed >>>> >>>> Here's what I understand: >>>> >>>> - For switches in dsa_switches_apply_workarounds[] >>>> - Enforce the latter for shared ports. >>>> - Enforce the former for user ports. >>>> >>>> - For switches not in dsa_switches_apply_workarounds[] >>>> - Enforce the former for all ports. >>> >>> No, no. We enforce the dt-schema regardless of switch presence in >>> dsa_switches_apply_workarounds[], to encourage users to fix device trees >>> (those who run schema validation). The kernel workaround consists in >>> doing something (skipping phylink) for the device trees where the schema >>> warns on shared ports. But there should be a single sub-schema for >>> validating phylink bindings, whatever port kind it is. >> >> Hmm, like writing phylink.yaml and then referring to it under the port >> pattern node? This could prevent a lot of repetition. >> >> Arınç > > Yes, that would sound good. If I understand correctly, these phylink rules are for switch ports. The fixed-link, phy-handle, and managed properties are described on ethernet-controller.yaml so I thought it would make sense to define the rules there and refer to them where they're needed. Example: diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml index 480120469953..7279ab31aea7 100644 --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml @@ -65,16 +65,8 @@ if: - required: [ ethernet ] - required: [ link ] then: - allOf: - - required: - - phy-mode - - oneOf: - - required: - - fixed-link - - required: - - phy-handle - - required: - - managed + $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch + required: [ phy-mode ] additionalProperties: true diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml index e532c6b795f4..742aaf1a5ef2 100644 --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml @@ -179,6 +179,15 @@ required: - compatible - reg +if: + required: [ mdio ] +then: + patternProperties: + "^(ethernet-)?ports$": + patternProperties: + "^(ethernet-)?port@[0-9]+$": + $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch + $defs: mt7530-dsa-port: patternProperties: diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml index 9f6a5ccbcefe..d7256f33d946 100644 --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml @@ -284,6 +284,21 @@ allOf: controllers that have configurable TX internal delays. If this property is present then the MAC applies the TX delay. +$defs: + phylink-switch: + description: phylink bindings for switch ports + allOf: + - anyOf: + - required: [ fixed-link ] + - required: [ phy-handle ] + - required: [ managed ] + + - if: + required: [ fixed-link ] + then: + not: + required: [ managed ] + additionalProperties: true ... Arınç
On Wed, Sep 13, 2023 at 01:59:17PM +0300, Arınç ÜNAL wrote: > If I understand correctly, these phylink rules are for switch ports. The > fixed-link, phy-handle, and managed properties are described on > ethernet-controller.yaml so I thought it would make sense to define the > rules there and refer to them where they're needed. > > Example: > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > index 480120469953..7279ab31aea7 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > @@ -65,16 +65,8 @@ if: > - required: [ ethernet ] > - required: [ link ] > then: > - allOf: > - - required: > - - phy-mode > - - oneOf: > - - required: > - - fixed-link > - - required: > - - phy-handle > - - required: > - - managed > + $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch > + required: [ phy-mode ] > additionalProperties: true > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > index e532c6b795f4..742aaf1a5ef2 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > @@ -179,6 +179,15 @@ required: > - compatible > - reg > +if: > + required: [ mdio ] > +then: > + patternProperties: > + "^(ethernet-)?ports$": > + patternProperties: > + "^(ethernet-)?port@[0-9]+$": > + $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink-switch > + > $defs: > mt7530-dsa-port: > patternProperties: > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml > index 9f6a5ccbcefe..d7256f33d946 100644 > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml > @@ -284,6 +284,21 @@ allOf: > controllers that have configurable TX internal delays. If this > property is present then the MAC applies the TX delay. > +$defs: > + phylink-switch: > + description: phylink bindings for switch ports > + allOf: > + - anyOf: > + - required: [ fixed-link ] > + - required: [ phy-handle ] > + - required: [ managed ] > + > + - if: > + required: [ fixed-link ] > + then: > + not: > + required: [ managed ] > + > additionalProperties: true > ... > > Arınç I don't think they're for switch ports only. Any driver which uses phylink_fwnode_phy_connect() or its derivatives gets subject to the same bindings. But putting the sub-schema in ethernet-controller.yaml makes sense, just maybe not naming it "phylink-switch".
On 13.09.2023 14:04, Vladimir Oltean wrote: > I don't think they're for switch ports only. Any driver which uses > phylink_fwnode_phy_connect() or its derivatives gets subject to the same > bindings. But putting the sub-schema in ethernet-controller.yaml makes > sense, just maybe not naming it "phylink-switch". Got it. Should we disallow managed altogether when fixed-link is also defined, or just with in-band-status value? Currently: diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml index 9f6a5ccbcefe..3b5946a4be34 100644 --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml @@ -284,6 +284,21 @@ allOf: controllers that have configurable TX internal delays. If this property is present then the MAC applies the TX delay. +$defs: + phylink: + description: phylink bindings for ethernet controllers + allOf: + - anyOf: + - required: [ fixed-link ] + - required: [ phy-handle ] + - required: [ managed ] + + - if: + required: [ fixed-link ] + then: + properties: + managed: false + additionalProperties: true ... Arınç
On 13.09.2023 04:21, Andrew Lunn wrote: >>> The marvell switch can have up to 2 MDIO busses. If i remember >>> correctly, there is also one switch which has one MDIO bus per port. >> >> I'm writing the json-schema for Marvell switches. I've checked a few >> devicetree source files on Linus's Linux tree, I only see two buses used at >> the most. > > Sorry, i was ambiguous. Its not a Marvell switch which can have one > MDIO bus per port. I don't remember which switch it is, and it might > be a pure switchdev switch, not a DSA switch. > >> The internal bus and another bus with >> marvell,mv88e6xxx-mdio-external. I've never seen a devicetree with >> marvell,mv88e6250 though. Could the switch that has one MDIO bus per port >> be this one? Also, is every bus of this switch a >> marvell,mv88e6xxx-mdio-external bus or, one internal bus and the remaining >> are marvell mv88e6xxx-mdio-external buses? > > Only the 6390 family has two busses. It has an internal MDIO bus with > the same register API as all the other switches. However, unlike the > other families, it is not exposed on pins. And the 6390 has a second > MDIO bus using a slight variant of the registers, which is connected > to the outside world via pins. This second bus then has a compatible > to separate it from the normal MDIO bus. OK, I will disallow the external mdio bus for the compatible strings other than marvell,mv88e6190. Arınç
On Wed, Sep 13, 2023 at 02:35:11PM +0300, Arınç ÜNAL wrote: > On 13.09.2023 14:04, Vladimir Oltean wrote: > > I don't think they're for switch ports only. Any driver which uses > > phylink_fwnode_phy_connect() or its derivatives gets subject to the same > > bindings. But putting the sub-schema in ethernet-controller.yaml makes > > sense, just maybe not naming it "phylink-switch". > > Got it. Should we disallow managed altogether when fixed-link is also > defined, or just with in-band-status value? Just with the "in-band-status" value - just like phylink_parse_mode() implies. If not possible, just leave that condition out.
On 13.09.2023 16:00, Vladimir Oltean wrote: > On Wed, Sep 13, 2023 at 02:35:11PM +0300, Arınç ÜNAL wrote: >> On 13.09.2023 14:04, Vladimir Oltean wrote: >>> I don't think they're for switch ports only. Any driver which uses >>> phylink_fwnode_phy_connect() or its derivatives gets subject to the same >>> bindings. But putting the sub-schema in ethernet-controller.yaml makes >>> sense, just maybe not naming it "phylink-switch". >> >> Got it. Should we disallow managed altogether when fixed-link is also >> defined, or just with in-band-status value? > > Just with the "in-band-status" value - just like phylink_parse_mode() > implies. If not possible, just leave that condition out. I can rewrite the property to allow values other than in-band-status. - if: required: [ fixed-link ] then: properties: managed: const: auto Arınç
On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote: > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote: > > On 12.09.2023 22:34, Vladimir Oltean wrote: > > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote: > > > > The phylink bindings for user ports I ended up making by looking up the > > > > existing devicetrees are different than the phylink bindings for the shared > > > > (CPU and DSA) ports currently enforced on all switches. > > > > > > > > My phylink bindings for user ports: > > > > > > > > allOf: > > > > - anyOf: > > > > - required: [ fixed-link ] > > > > - required: [ phy-handle ] > > > > - required: [ managed ] > > > > > > > > - if: > > > > required: [ fixed-link ] > > > > then: > > > > not: > > > > required: [ managed ] > > > > > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug > > > which should be fixed. It's the same phylink that gets used in both cases, > > > user ports and shared ports :) > > > > One more thing, I don't recall phy-mode being required to be defined for > > user ports as it will default to GMII. I don't believe this is the same > > case for shared ports so phy-mode is required only for them? > > phy-mode is not strictly required, but I think there is a strong > preference to set it. IIRC, when looking at the DSA device trees, there > was no case where phy-mode would be absent on CPU/DSA ports if the other > link properties were also present, so we required it too. There were no > complaints in 1 year since dsa_shared_port_validate_of() is there. The > requirement can be relaxed to just a warning and no error in the kernel, > and the removal of "required" in the schema, if it helps making it > common with user ports. However, phylink pretty much requires phy-mode to be specified to be something sane for shared ports, so I wouldn't be in favour of relaxing the checkinng in dsa_shared_port_validate_of()... not unless you're now going to accept the approach I originally proposed to have DSA drivers tell the core (and thus phylink) what phy-mode and other link parameters should be used when they are missing from DT.
On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote: > However, phylink pretty much requires phy-mode to be specified to be > something sane for shared ports, so I wouldn't be in favour of relaxing > the checkinng in dsa_shared_port_validate_of()... not unless you're > now going to accept the approach I originally proposed to have DSA > drivers tell the core (and thus phylink) what phy-mode and other link > parameters should be used when they are missing from DT. Ok, so with a missing phy-mode on the CPU port, phylink_parse_fixedlink() -> phy_lookup_setting() will return NULL and that will print a phylink_warn(), but other than that, phylink_mac_link_up() does get called at the right speed and duplex. I agree that for sane behavior it should be specified, but it appears that even with PHY_INTERFACE_MODE_NA something can be hacked up... [ 4.818368] sja1105 spi0.1: Failed to read phy-mode or phy-interface-type property for port 4 [ 4.864667] sja1105 spi0.1: OF node /soc/spi@2100000/ethernet-switch@1/ports/port@4 of CPU port 4 lacks the required "phy-mode" property [ 4.882957] sja1105 spi0.1: pl->link_config.speed 1000 pl->link_config.duplex 1 pl->supported 00,00000000,00000000,00000240 [ 4.894189] sja1105 spi0.1: phy_setting speed -1 duplex -1 bit -1 [ 4.900283] sja1105 spi0.1: fixed link full duplex 1000Mbps not recognised [ 4.907798] sja1105 spi0.1: configuring for fixed/ link mode [ 4.916183] sja1105 spi0.1 swp5 (uninitialized): PHY [mdio@2d24000:06] driver [Broadcom BCM5464] (irq=POLL) [ 4.934770] sja1105 spi0.1 swp2 (uninitialized): PHY [mdio@2d24000:03] driver [Broadcom BCM5464] (irq=POLL) [ 4.951619] sja1105 spi0.1 swp3 (uninitialized): PHY [mdio@2d24000:04] driver [Broadcom BCM5464] (irq=POLL) [ 4.968349] sja1105 spi0.1 swp4 (uninitialized): PHY [mdio@2d24000:05] driver [Broadcom BCM5464] (irq=POLL) [ 4.984017] fsl-gianfar soc:ethernet@2d90000 eth2: entered promiscuous mode [ 4.991327] DSA: tree 0 setup [ 4.995129] sja1105 spi0.1: sja1105_mac_link_up: port 4 interface speed 1000 duplex 1 [ 5.005004] sja1105 spi0.1: Link is Up - 1Gbps/Full - flow control off diff --git a/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts b/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts index 1ea32fff4120..0bfffcb51af9 100644 --- a/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts +++ b/arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts @@ -90,7 +90,7 @@ port@3 { port@4 { /* Internal port connected to eth2 */ ethernet = <&enet2>; - phy-mode = "rgmii"; +// phy-mode = "rgmii"; rx-internal-delay-ps = <0>; tx-internal-delay-ps = <0>; reg = <4>; diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index a23d980d28f5..dba1fa545a9c 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -327,6 +327,8 @@ static int sja1105_init_mii_settings(struct sja1105_private *priv) mii->xmii_mode[i] = XMII_MODE_SGMII; mii->special[i] = true; break; + case PHY_INTERFACE_MODE_NA: + break; unsupported: default: dev_err(dev, "Unsupported PHY mode %s on port %d!\n", @@ -1205,11 +1207,10 @@ static int sja1105_parse_ports_node(struct sja1105_private *priv, /* Get PHY mode from DT */ err = of_get_phy_mode(child, &phy_mode); if (err) { - dev_err(dev, "Failed to read phy-mode or " + dev_warn(dev, "Failed to read phy-mode or " "phy-interface-type property for port %d\n", index); - of_node_put(child); - return -ENODEV; + phy_mode = PHY_INTERFACE_MODE_NA; } phy_node = of_parse_phandle(child, "phy-handle", 0); @@ -1383,6 +1384,8 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port, { struct sja1105_private *priv = ds->priv; + dev_err(ds->dev, "%s: port %d interface %s speed %d duplex %d\n", __func__, port, phy_modes(interface), speed, duplex); + sja1105_adjust_port_config(priv, port, speed); sja1105_inhibit_tx(priv, BIT(port), false); @@ -1414,7 +1417,10 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port, * config (the xMII Mode table cannot be dynamically * reconfigured), and we have to program that early. */ - __set_bit(phy_mode, config->supported_interfaces); + if (phy_mode == PHY_INTERFACE_MODE_NA) + phy_interface_set_rgmii(config->supported_interfaces); + else + __set_bit(phy_mode, config->supported_interfaces); } /* The MAC does not support pause frames, and also doesn't diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 0d7354955d62..674689011059 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -841,6 +841,15 @@ static int phylink_parse_fixedlink(struct phylink *pl, if (autoneg) phylink_set(pl->supported, Autoneg); + phylink_err(pl, "pl->link_config.speed %d pl->link_config.duplex %d pl->supported %*pb\n", + pl->link_config.speed, pl->link_config.duplex, __ETHTOOL_LINK_MODE_MASK_NBITS, + pl->supported); + + phylink_err(pl, "phy_setting speed %d duplex %d bit %d\n", + s ? s->speed : -1, + s ? s->duplex : -1, + s ? s->bit : -1); + if (s) { __set_bit(s->bit, pl->supported); __set_bit(s->bit, pl->link_config.lp_advertising); diff --git a/net/dsa/port.c b/net/dsa/port.c index 5f01bd4f9dec..34e5dc48f0ff 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1927,6 +1927,16 @@ static const char * const dsa_switches_apply_workarounds[] = { #if IS_ENABLED(CONFIG_NET_DSA_SMSC_LAN9303_I2C) "smsc,lan9303-i2c", #endif + "nxp,sja1105e", + "nxp,sja1105t", + "nxp,sja1105p", + "nxp,sja1105q", + "nxp,sja1105r", + "nxp,sja1105s", + "nxp,sja1110a", + "nxp,sja1110b", + "nxp,sja1110c", + "nxp,sja1110d", NULL, };
On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote: > On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote: > > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote: > > > On 12.09.2023 22:34, Vladimir Oltean wrote: > > > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote: > > > > > The phylink bindings for user ports I ended up making by looking up the > > > > > existing devicetrees are different than the phylink bindings for the shared > > > > > (CPU and DSA) ports currently enforced on all switches. > > > > > > > > > > My phylink bindings for user ports: > > > > > > > > > > allOf: > > > > > - anyOf: > > > > > - required: [ fixed-link ] > > > > > - required: [ phy-handle ] > > > > > - required: [ managed ] > > > > > > > > > > - if: > > > > > required: [ fixed-link ] > > > > > then: > > > > > not: > > > > > required: [ managed ] > > > > > > > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug > > > > which should be fixed. It's the same phylink that gets used in both cases, > > > > user ports and shared ports :) > > > > > > One more thing, I don't recall phy-mode being required to be defined for > > > user ports as it will default to GMII. I don't believe this is the same > > > case for shared ports so phy-mode is required only for them? > > > > phy-mode is not strictly required, but I think there is a strong > > preference to set it. IIRC, when looking at the DSA device trees, there > > was no case where phy-mode would be absent on CPU/DSA ports if the other > > link properties were also present, so we required it too. There were no > > complaints in 1 year since dsa_shared_port_validate_of() is there. The > > requirement can be relaxed to just a warning and no error in the kernel, > > and the removal of "required" in the schema, if it helps making it > > common with user ports. > > However, phylink pretty much requires phy-mode to be specified to be > something sane for shared ports, so I wouldn't be in favour of relaxing > the checkinng in dsa_shared_port_validate_of()... not unless you're > now going to accept the approach I originally proposed to have DSA > drivers tell the core (and thus phylink) what phy-mode and other link > parameters should be used when they are missing from DT. You mean the approach that I picked up using software nodes that got thrown out by the software node people? That approach that I picked up from you and tried to get merged? No, that's not going to happen, and it's not a question of whether _I_ am going to accept that approach or not. So don't throw that back on me, please. If this is something that we want to solve, we need to stop being so devisive (your language above is so) and try to come up with a solution that is acceptable to everyone... the swnode approach doesn't seem to be it.
On Thu, Sep 14, 2023 at 07:06:11PM +0100, Russell King (Oracle) wrote: > On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote: > > On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote: > > > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote: > > > > On 12.09.2023 22:34, Vladimir Oltean wrote: > > > > > On Tue, Sep 12, 2023 at 10:23:51PM +0300, Arınç ÜNAL wrote: > > > > > > The phylink bindings for user ports I ended up making by looking up the > > > > > > existing devicetrees are different than the phylink bindings for the shared > > > > > > (CPU and DSA) ports currently enforced on all switches. > > > > > > > > > > > > My phylink bindings for user ports: > > > > > > > > > > > > allOf: > > > > > > - anyOf: > > > > > > - required: [ fixed-link ] > > > > > > - required: [ phy-handle ] > > > > > > - required: [ managed ] > > > > > > > > > > > > - if: > > > > > > required: [ fixed-link ] > > > > > > then: > > > > > > not: > > > > > > required: [ managed ] > > > > > > > > > > Right, it should have been anyOf and not oneOf.. my mistake. It is a bug > > > > > which should be fixed. It's the same phylink that gets used in both cases, > > > > > user ports and shared ports :) > > > > > > > > One more thing, I don't recall phy-mode being required to be defined for > > > > user ports as it will default to GMII. I don't believe this is the same > > > > case for shared ports so phy-mode is required only for them? > > > > > > phy-mode is not strictly required, but I think there is a strong > > > preference to set it. IIRC, when looking at the DSA device trees, there > > > was no case where phy-mode would be absent on CPU/DSA ports if the other > > > link properties were also present, so we required it too. There were no > > > complaints in 1 year since dsa_shared_port_validate_of() is there. The > > > requirement can be relaxed to just a warning and no error in the kernel, > > > and the removal of "required" in the schema, if it helps making it > > > common with user ports. > > > > However, phylink pretty much requires phy-mode to be specified to be > > something sane for shared ports, so I wouldn't be in favour of relaxing > > the checkinng in dsa_shared_port_validate_of()... not unless you're > > now going to accept the approach I originally proposed to have DSA > > drivers tell the core (and thus phylink) what phy-mode and other link > > parameters should be used when they are missing from DT. > > You mean the approach that I picked up using software nodes that got > thrown out by the software node people? That approach that I picked > up from you and tried to get merged? > > No, that's not going to happen, and it's not a question of whether > _I_ am going to accept that approach or not. So don't throw that > back on me, please. > > If this is something that we want to solve, we need to stop being so > devisive (your language above is so) and try to come up with a > solution that is acceptable to everyone... the swnode approach > doesn't seem to be it. Oh dear. I must be going mad!
Hi Russell, On Thu, Sep 14, 2023 at 07:07:13PM +0100, Russell King (Oracle) wrote: > On Thu, Sep 14, 2023 at 07:06:11PM +0100, Russell King (Oracle) wrote: > > On Wed, Sep 13, 2023 at 04:59:19PM +0100, Russell King (Oracle) wrote: > > > On Wed, Sep 13, 2023 at 10:42:31AM +0300, Vladimir Oltean wrote: > > > > On Wed, Sep 13, 2023 at 08:52:37AM +0300, Arınç ÜNAL wrote: > > > > > One more thing, I don't recall phy-mode being required to be defined for > > > > > user ports as it will default to GMII. I don't believe this is the same > > > > > case for shared ports so phy-mode is required only for them? > > > > > > > > phy-mode is not strictly required, but I think there is a strong > > > > preference to set it. IIRC, when looking at the DSA device trees, there > > > > was no case where phy-mode would be absent on CPU/DSA ports if the other > > > > link properties were also present, so we required it too. There were no > > > > complaints in 1 year since dsa_shared_port_validate_of() is there. The > > > > requirement can be relaxed to just a warning and no error in the kernel, > > > > and the removal of "required" in the schema, if it helps making it > > > > common with user ports. > > > > > > However, phylink pretty much requires phy-mode to be specified to be > > > something sane for shared ports, so I wouldn't be in favour of relaxing > > > the checkinng in dsa_shared_port_validate_of()... not unless you're > > > now going to accept the approach I originally proposed to have DSA > > > drivers tell the core (and thus phylink) what phy-mode and other link > > > parameters should be used when they are missing from DT. > > > > You mean the approach that I picked up using software nodes that got > > thrown out by the software node people? That approach that I picked > > up from you and tried to get merged? > > > > No, that's not going to happen, and it's not a question of whether > > _I_ am going to accept that approach or not. So don't throw that > > back on me, please. > > > > If this is something that we want to solve, we need to stop being so > > devisive (your language above is so) and try to come up with a > > solution that is acceptable to everyone... the swnode approach > > doesn't seem to be it. > > Oh dear. I must be going mad! So first things first: I am not advocating for making phy-mode fully optional in the sense that you say (if absent, then write non-OF code through which DSA infers the phy-mode from drivers). I'm happy with the current form of the code. I was just trying to add some nuance to this bizarre aspect signalled by Arınç - phy-mode is not required for user ports, presumably because when it is absent, user ports will default to GMII. That isn't an intrinsic feature of user ports, but rather of having a phydev, and so, because DSA/CPU ports can also have a phydev, logically it means that phy-mode can also be omitted in that particular case, with the same result. Our missing_phy_mode check from dsa_shared_port_validate_of() is theoretically more restrictive than it needs to be, because it artificially prohibits that behavior, and it results in an inexplicable difference in the phylink dt-bindings for user vs shared ports. So that's where my relaxation proposal came from: we could make missing_phy_mode non-fatal, and that would permit the configurations which can work to work, and the ones which can't work will fail elsewhere. Just like for the user ports. Where I wasn't absolutely clear is that I don't want Arınç to change any of that. Right now, on DSA shared ports, phy-mode is required and on user ports it isn't. The difference is a bit strange (arbitrary considering the example above) and should maybe be settled at some point in the future, but for now, the dt-bindings should document it like that.
diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml index ec74a660beda..03ccedbc49dc 100644 --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml @@ -31,6 +31,24 @@ properties: (single device hanging off a CPU port) must not specify this property $ref: /schemas/types.yaml#/definitions/uint32-array + mdio: + description: The internal MDIO bus of the switch + $ref: /schemas/net/mdio.yaml# + +if: + required: [ mdio ] +then: + patternProperties: + "^(ethernet-)?ports$": + patternProperties: + "^(ethernet-)?port@[0-9]+$": + if: + not: + required: [ ethernet ] + then: + required: + - phy-handle + additionalProperties: true $defs: diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml index cfd69c2604ea..ea7db0890abc 100644 --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# title: Realtek switches for unmanaged switches -allOf: - - $ref: dsa.yaml#/$defs/ethernet-ports - maintainers: - Linus Walleij <linus.walleij@linaro.org> @@ -95,37 +92,38 @@ properties: - '#address-cells' - '#interrupt-cells' - mdio: - $ref: /schemas/net/mdio.yaml# - unevaluatedProperties: false - - properties: - compatible: - const: realtek,smi-mdio - -if: - required: - - reg - -then: - $ref: /schemas/spi/spi-peripheral-props.yaml# - not: - required: - - mdc-gpios - - mdio-gpios - - mdio - - properties: - mdc-gpios: false - mdio-gpios: false - mdio: false - -else: - required: - - mdc-gpios - - mdio-gpios - - mdio - - reset-gpios +allOf: + - $ref: dsa.yaml#/$defs/ethernet-ports + - if: + required: [ mdio ] + then: + properties: + mdio: + properties: + compatible: + const: realtek,smi-mdio + + - if: + required: + - reg + then: + $ref: /schemas/spi/spi-peripheral-props.yaml# + not: + required: + - mdc-gpios + - mdio-gpios + - mdio + + properties: + mdc-gpios: false + mdio-gpios: false + mdio: false + else: + required: + - mdc-gpios + - mdio-gpios + - mdio + - reset-gpios required: - compatible
Add the schema to document the internal MDIO bus. Adjust realtek.yaml accordingly. Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> --- .../devicetree/bindings/net/dsa/dsa.yaml | 18 +++++ .../devicetree/bindings/net/dsa/realtek.yaml | 66 +++++++++---------- 2 files changed, 50 insertions(+), 34 deletions(-)