Hello there.
This patch series documents the internal MDIO bus of the switches
controlled by DSA and brings support for it on the MT7530 DSA subdriver.
There are also some dt-bindings improvements included.
Cheers.
Arınç
From: David Bauer <[email protected]>
The MT753x switches provide a switch-internal MDIO bus for the embedded
PHYs.
Register a OF sub-node on the switch OF-node for this internal MDIO bus.
This allows to configure the embedded PHYs using device-tree.
Signed-off-by: David Bauer <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8fbda739c1b3..9bb805de397a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2152,10 +2152,13 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
{
struct dsa_switch *ds = priv->ds;
struct device *dev = priv->dev;
+ struct device_node *np, *mnp;
struct mii_bus *bus;
static int idx;
int ret;
+ np = priv->dev->of_node;
+
bus = devm_mdiobus_alloc(dev);
if (!bus)
return -ENOMEM;
@@ -2174,7 +2177,9 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
if (priv->irq)
mt7530_setup_mdio_irq(priv);
- ret = devm_mdiobus_register(dev, bus);
+ mnp = of_get_child_by_name(np, "mdio");
+ ret = devm_of_mdiobus_register(dev, bus, mnp);
+ of_node_put(mnp);
if (ret) {
dev_err(dev, "failed to register MDIO bus: %d\n", ret);
if (priv->irq)
--
2.39.2
The port@5 node on the example is missing the ethernet property. Add it.
Remove the MAC bindings on the example as they cannot be validated.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
.../bindings/net/dsa/microchip,lan937x.yaml | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 8d7e878b84dc..49af4b0d5916 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -68,16 +68,6 @@ examples:
- |
#include <dt-bindings/gpio/gpio.h>
- macb0 {
- #address-cells = <1>;
- #size-cells = <0>;
-
- fixed-link {
- speed = <1000>;
- full-duplex;
- };
- };
-
spi {
#address-cells = <1>;
#size-cells = <0>;
@@ -138,6 +128,7 @@ examples:
phy-mode = "rgmii";
tx-internal-delay-ps = <2000>;
rx-internal-delay-ps = <2000>;
+ ethernet = <&macb1>;
fixed-link {
speed = <1000>;
--
2.39.2
Add the schema to document the internal MDIO bus. Adjust realtek.yaml
accordingly.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
.../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 ]
+ 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 <[email protected]>
@@ -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
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 <[email protected]>
> ---
> .../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 <[email protected]>
>
> @@ -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 Sat, Aug 12, 2023 at 12:17:05PM +0300, Arınç ÜNAL wrote:
> The port@5 node on the example is missing the ethernet property. Add it.
> Remove the MAC bindings on the example as they cannot be validated.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> .../bindings/net/dsa/microchip,lan937x.yaml | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> index 8d7e878b84dc..49af4b0d5916 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> @@ -68,16 +68,6 @@ examples:
> - |
> #include <dt-bindings/gpio/gpio.h>
>
> - macb0 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - fixed-link {
> - speed = <1000>;
> - full-duplex;
> - };
> - };
> -
> spi {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -138,6 +128,7 @@ examples:
> phy-mode = "rgmii";
> tx-internal-delay-ps = <2000>;
> rx-internal-delay-ps = <2000>;
> + ethernet = <&macb1>;
macb1 instead of macb0: was it intentional?
>
> fixed-link {
> speed = <1000>;
> --
> 2.39.2
>
On 13.08.2023 14:07, Vladimir Oltean wrote:
> On Sat, Aug 12, 2023 at 12:17:05PM +0300, Arınç ÜNAL wrote:
>> The port@5 node on the example is missing the ethernet property. Add it.
>> Remove the MAC bindings on the example as they cannot be validated.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>> .../bindings/net/dsa/microchip,lan937x.yaml | 11 +----------
>> 1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> index 8d7e878b84dc..49af4b0d5916 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
>> @@ -68,16 +68,6 @@ examples:
>> - |
>> #include <dt-bindings/gpio/gpio.h>
>>
>> - macb0 {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> -
>> - fixed-link {
>> - speed = <1000>;
>> - full-duplex;
>> - };
>> - };
>> -
>> spi {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> @@ -138,6 +128,7 @@ examples:
>> phy-mode = "rgmii";
>> tx-internal-delay-ps = <2000>;
>> rx-internal-delay-ps = <2000>;
>> + ethernet = <&macb1>;
>
> macb1 instead of macb0: was it intentional?
Yes, port@4 defines macb0. I used macb1 for port@5 here.
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 <[email protected]>
>> ---
>> .../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
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?
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 Sat, Aug 12, 2023 at 12:17:05PM +0300, Arınç ÜNAL wrote:
> The port@5 node on the example is missing the ethernet property. Add it.
> Remove the MAC bindings on the example as they cannot be validated.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
> .../bindings/net/dsa/microchip,lan937x.yaml | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> index 8d7e878b84dc..49af4b0d5916 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> @@ -68,16 +68,6 @@ examples:
> - |
> #include <dt-bindings/gpio/gpio.h>
>
> - macb0 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - fixed-link {
> - speed = <1000>;
> - full-duplex;
> - };
> - };
> -
> spi {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -138,6 +128,7 @@ examples:
> phy-mode = "rgmii";
> tx-internal-delay-ps = <2000>;
> rx-internal-delay-ps = <2000>;
> + ethernet = <&macb1>;
>
> fixed-link {
> speed = <1000>;
> --
> 2.39.2
>
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 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 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 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 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 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 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 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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 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 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!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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.