Hello there.
This patch series defines phylink bindings and enforces them for the
ethernet controllers that need them.
Some schemas had to be changed to properly enforce phylink bindings for all
of the affected ethernet controllers. Some of the documents of these
ethernet controllers were non json-schema, which had to be converted.
I will convert the remaining documents to json-schema while this patch
series receives reviews.
Cheers.
Arınç
v2:
After many discussions on v1, the concept of the patch series changed a lot
to document the changes from v1.
Link to v1:
https://lore.kernel.org/netdev/[email protected]/
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 035a34b50f31..7db9af7b9658 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2156,10 +2156,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;
@@ -2178,7 +2181,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 dt-bindings document was converted to json-schema. Update the file
name.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 612d6d1dbf36..5d4e6b578c04 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12564,7 +12564,7 @@ MARVELL 88E6XXX ETHERNET SWITCH FABRIC DRIVER
M: Andrew Lunn <[email protected]>
L: [email protected]
S: Maintained
-F: Documentation/devicetree/bindings/net/dsa/marvell.txt
+F: Documentation/devicetree/bindings/net/dsa/marvell.yaml
F: Documentation/networking/devlink/mv88e6xxx.rst
F: drivers/net/dsa/mv88e6xxx/
F: include/linux/dsa/mv88e6xxx.h
--
2.39.2
Phylink bindings are required for ethernet controllers that utilise
phylink_fwnode_phy_connect() directly or through phylink_of_phy_connect(),
and register OF-based only MDIO buses, if they register any.
All the drivers that utilise phylink_fwnode_phy_connect() directly or
through phylink_of_phy_connect():
- DSA
- drivers/net/ethernet/mscc/ocelot_net.c
- mscc,vsc7514-switch.yaml
- drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
- microchip,sparx5-switch.yaml
- drivers/net/ethernet/altera/altera_tse_main.c
- altr,tse.yaml
- drivers/net/ethernet/xilinx/xilinx_axienet_main.c
- xlnx,axi-ethernet.yaml
- drivers/net/ethernet/mediatek/mtk_eth_soc.c
- mediatek,net.yaml
- drivers/net/ethernet/ti/am65-cpsw-nuss.c
- ti,k3-am654-cpsw-nuss.yaml
- drivers/net/ethernet/atheros/ag71xx.c
- qca,ar71xx.yaml
- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
- fsl,fman-dtsec.yaml
- drivers/net/ethernet/microchip/lan966x/lan966x_main.c
- microchip,lan966x-switch.yaml
- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
- marvell,pp2.yaml
- drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
- fsl,qoriq-mc-dpmac.yaml
- drivers/net/ethernet/cadence/macb_main.c
- cdns,macb.yaml
- Can register non-OF-based bus.
- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
- snps,dwmac.yaml
- Can register non-OF-based bus.
- drivers/net/ethernet/marvell/mvneta.c
- marvell-armada-370-neta.txt
- drivers/net/ethernet/freescale/enetc/enetc.c
- fsl-enetc.txt
RFC: The drivers marked with "can register non-OF-based bus" seem to search
the MDIO bus to connect the PHY to the MAC using phylink_connect_phy()
and/or phy_find_first() if phylink bindings don't exist. Should we enforce
phylink bindings on their schemas regardless?
DSA, like any other driver utilising the Linux MDIO infrastructure,
can register a bus. On certain conditions, it registers the MDIO
bus of the switch it controls non-OF-based.
We can know when DSA won't create any non-OF-based buses. That leaves us
with only OF-based buses in which case we can enforce phylink bindings for
user ports. The bindings are already enforced for shared ports so we can
enforce the bindings for all ports on the switch schemas which will achieve
the same result with fewer lines.
By looking at whether the mdio child node exists and what is explained on
it, we can enforce phylink bindings.
- mscc,vsc7514-switch.yaml
- Enforce phylink bindings for all ports.
- arrow,xrs700x.yaml
- Enforce phylink bindings for all ports.
- brcm,b53.yaml
- Enforce phylink bindings for all ports if "mdio" is defined.
- brcm,sf2.yaml
- Enforce phylink bindings for all ports.
- hirschmann,hellcreek.yaml
- Enforce phylink bindings for all ports.
- mediatek,mt7530.yaml
- Enforce phylink bindings for all ports if "mdio" is defined.
- microchip,ksz.yaml
- Enforce phylink bindings for all ports if "mdio" is defined.
- microchip,lan937x.yaml
- Enforce phylink bindings for all ports if "mdio" is defined.
- mscc,ocelot.yaml
- Enforce phylink bindings for all ports.
- nxp,sja1105.yaml
- Enforce phylink bindings for all ports.
- qca8k.yaml
- Enforce phylink bindings for all ports if "mdio" is defined.
- realtek.yaml
- Enforce phylink bindings for all ports if "mdio" is defined.
- renesas,rzn1-a5psw.yaml
- Enforce phylink bindings for all ports.
- ar9331.txt
- Enforce phylink bindings for all ports.
- lan9303.txt
- Enforce phylink bindings for all ports if "mdio" is defined.
- lantiq-gswip.txt
- Enforce phylink bindings for all ports.
- marvell.txt
- Enforce phylink bindings for all ports if "mdio" is defined.
- vitesse,vsc73xx.txt
- Enforce phylink bindings for all ports if "mdio" is defined.
I will convert the non json-schema documents accordingly.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
.../devicetree/bindings/net/altr,tse.yaml | 1 +
.../devicetree/bindings/net/cdns,macb.yaml | 1 +
.../bindings/net/dsa/arrow,xrs700x.yaml | 6 ++
.../devicetree/bindings/net/dsa/brcm,b53.yaml | 9 +++
.../devicetree/bindings/net/dsa/brcm,sf2.yaml | 6 +-
.../net/dsa/hirschmann,hellcreek.yaml | 6 ++
.../bindings/net/dsa/mediatek,mt7530.yaml | 9 +++
.../bindings/net/dsa/microchip,ksz.yaml | 9 +++
.../bindings/net/dsa/microchip,lan937x.yaml | 9 +++
.../bindings/net/dsa/mscc,ocelot.yaml | 6 ++
.../bindings/net/dsa/nxp,sja1105.yaml | 1 +
.../devicetree/bindings/net/dsa/qca8k.yaml | 9 +++
.../devicetree/bindings/net/dsa/realtek.yaml | 57 +++++++++++--------
.../bindings/net/dsa/renesas,rzn1-a5psw.yaml | 2 +-
.../bindings/net/fsl,fman-dtsec.yaml | 1 +
.../bindings/net/fsl,qoriq-mc-dpmac.yaml | 1 +
.../devicetree/bindings/net/marvell,pp2.yaml | 4 +-
.../devicetree/bindings/net/mediatek,net.yaml | 1 +
.../net/microchip,lan966x-switch.yaml | 4 +-
.../bindings/net/microchip,sparx5-switch.yaml | 5 +-
.../bindings/net/mscc,vsc7514-switch.yaml | 5 ++
.../devicetree/bindings/net/qca,ar71xx.yaml | 1 +
.../devicetree/bindings/net/snps,dwmac.yaml | 1 +
.../bindings/net/ti,k3-am654-cpsw-nuss.yaml | 4 +-
.../bindings/net/xlnx,axi-ethernet.yaml | 3 +-
25 files changed, 130 insertions(+), 31 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/altr,tse.yaml b/Documentation/devicetree/bindings/net/altr,tse.yaml
index f5d3b70af07a..24e0f1213109 100644
--- a/Documentation/devicetree/bindings/net/altr,tse.yaml
+++ b/Documentation/devicetree/bindings/net/altr,tse.yaml
@@ -67,6 +67,7 @@ required:
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
- if:
properties:
compatible:
diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index bf8894a0257e..a455de04c5f1 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -156,6 +156,7 @@ required:
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
- if:
not:
diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
index 9565a7402146..7829fc9e202d 100644
--- a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
@@ -29,6 +29,12 @@ properties:
reg:
maxItems: 1
+patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
index 0bb2ff0cf2d0..ed5b3eab1948 100644
--- a/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
@@ -142,6 +142,15 @@ allOf:
reg:
maxItems: 1
+ - if:
+ required: [ mdio ]
+ then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
unevaluatedProperties: false
examples:
diff --git a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
index c745407f2f68..6cffcd35a943 100644
--- a/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/brcm,sf2.yaml
@@ -81,7 +81,10 @@ properties:
patternProperties:
'^port@[0-9a-f]$':
- $ref: dsa-port.yaml#
+ type: object
+ allOf:
+ - $ref: dsa-port.yaml#
+ - $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
unevaluatedProperties: false
properties:
@@ -161,6 +164,7 @@ examples:
port@0 {
label = "gphy";
reg = <0>;
+ phy-handle = <&phy8>;
};
};
};
diff --git a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
index 4021b054f684..30b4c7b1fd08 100644
--- a/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml
@@ -67,6 +67,12 @@ properties:
additionalProperties: false
+patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index 31a7dbbf704d..a8e1134707e4 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -297,6 +297,15 @@ allOf:
mediatek,mcm: false
reset-names: false
+ - if:
+ required: [ mdio ]
+ then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
unevaluatedProperties: false
examples:
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 20e4174fe1ab..d8f12234eec6 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -60,6 +60,15 @@ required:
- compatible
- reg
+if:
+ required: [ mdio ]
+then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
unevaluatedProperties: false
examples:
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 33a4926b2d94..44a892b93eca 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -65,6 +65,15 @@ required:
- compatible
- reg
+if:
+ required: [ mdio ]
+then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
unevaluatedProperties: false
examples:
diff --git a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
index fe02d05196e4..80cc9c49282b 100644
--- a/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mscc,ocelot.yaml
@@ -73,6 +73,12 @@ properties:
little-endian: true
big-endian: true
+patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 6838dc165d06..1d5c4a81caf6 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -79,6 +79,7 @@ patternProperties:
"^(ethernet-)?ports$":
patternProperties:
"^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
allOf:
- if:
properties:
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index c472050582be..546939ab2053 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -114,6 +114,15 @@ required:
- compatible
- reg
+if:
+ required: [ mdio ]
+then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-6]$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
unevaluatedProperties: false
examples:
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 62ebaa4b5ae3..ec0428b74157 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -6,8 +6,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
title: Realtek switches for unmanaged switches
-allOf:
- - $ref: dsa.yaml#/$defs/ethernet-ports
+$ref: dsa.yaml#/$defs/ethernet-ports
maintainers:
- Linus Walleij <[email protected]>
@@ -107,33 +106,43 @@ properties:
compatible:
const: realtek,smi-mdio
-if:
- required:
- - reg
+allOf:
+ - if:
+ required:
+ - reg
-then:
- $ref: /schemas/spi/spi-peripheral-props.yaml#
- properties:
- mdc-gpios: false
- mdio-gpios: false
- mdio:
+ then:
+ $ref: /schemas/spi/spi-peripheral-props.yaml#
properties:
- compatible: false
+ mdc-gpios: false
+ mdio-gpios: false
+ mdio:
+ properties:
+ compatible: false
-else:
- properties:
- mdio:
- required:
- - compatible
+ else:
+ properties:
+ mdio:
+ required:
+ - compatible
- required:
- - mdc-gpios
- - mdio-gpios
- - mdio
- - reset-gpios
+ required:
+ - mdc-gpios
+ - mdio-gpios
+ - mdio
+ - reset-gpios
-required:
- - compatible
+ required:
+ - compatible
+
+ - if:
+ required: [ mdio ]
+ then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
unevaluatedProperties: false
diff --git a/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml b/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
index 9ad9f5cdf688..7ec4371932ef 100644
--- a/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
@@ -72,7 +72,7 @@ properties:
patternProperties:
"^(ethernet-)?port@[0-4]$":
- type: object
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
description: Ethernet switch ports
properties:
diff --git a/Documentation/devicetree/bindings/net/fsl,fman-dtsec.yaml b/Documentation/devicetree/bindings/net/fsl,fman-dtsec.yaml
index c80c880a9dab..c31db5f7d548 100644
--- a/Documentation/devicetree/bindings/net/fsl,fman-dtsec.yaml
+++ b/Documentation/devicetree/bindings/net/fsl,fman-dtsec.yaml
@@ -136,6 +136,7 @@ dependencies:
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
- if:
properties:
compatible:
diff --git a/Documentation/devicetree/bindings/net/fsl,qoriq-mc-dpmac.yaml b/Documentation/devicetree/bindings/net/fsl,qoriq-mc-dpmac.yaml
index a1b71b35319e..b2cab0fa7b35 100644
--- a/Documentation/devicetree/bindings/net/fsl,qoriq-mc-dpmac.yaml
+++ b/Documentation/devicetree/bindings/net/fsl,qoriq-mc-dpmac.yaml
@@ -15,6 +15,7 @@ description:
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
properties:
compatible:
diff --git a/Documentation/devicetree/bindings/net/marvell,pp2.yaml b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
index 4eadafc43d4f..8e60cc4bbbc9 100644
--- a/Documentation/devicetree/bindings/net/marvell,pp2.yaml
+++ b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
@@ -59,7 +59,9 @@ patternProperties:
'^(ethernet-)?port@[0-2]$':
type: object
description: subnode for each ethernet port.
- $ref: ethernet-controller.yaml#
+ allOf:
+ - $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
unevaluatedProperties: false
properties:
diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
index e74502a0afe8..7562bb64baa9 100644
--- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
+++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
@@ -391,6 +391,7 @@ patternProperties:
unevaluatedProperties: false
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
description:
Ethernet MAC node
properties:
diff --git a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
index 306ef9ecf2b9..a770cfc490a9 100644
--- a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
+++ b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
@@ -73,7 +73,9 @@ properties:
"^port@[0-9a-f]+$":
type: object
- $ref: /schemas/net/ethernet-controller.yaml#
+ allOf:
+ - $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
unevaluatedProperties: false
properties:
diff --git a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml
index fcafef8d5a33..da4b4d934705 100644
--- a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml
+++ b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml
@@ -84,7 +84,10 @@ properties:
patternProperties:
"^port@[0-9a-f]+$":
- $ref: /schemas/net/ethernet-controller.yaml#
+ type: object
+ allOf:
+ - $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
unevaluatedProperties: false
properties:
diff --git a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
index 8ee2c7d7ff42..b3aff7b0790e 100644
--- a/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
+++ b/Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml
@@ -126,6 +126,11 @@ properties:
- const: xtr
- const: fdma
+ ethernet-ports:
+ patternProperties:
+ "^port@[0-9a-f]+$":
+ $ref: ethernet-controller.yaml#/$defs/phylink
+
required:
- compatible
- reg
diff --git a/Documentation/devicetree/bindings/net/qca,ar71xx.yaml b/Documentation/devicetree/bindings/net/qca,ar71xx.yaml
index 89f94b31b546..d021bfb4ad20 100644
--- a/Documentation/devicetree/bindings/net/qca,ar71xx.yaml
+++ b/Documentation/devicetree/bindings/net/qca,ar71xx.yaml
@@ -8,6 +8,7 @@ title: QCA AR71XX MAC
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
maintainers:
- Oleksij Rempel <[email protected]>
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index ddf9522a5dc2..62d8c2cfafe9 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -567,6 +567,7 @@ dependencies:
allOf:
- $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
- if:
properties:
compatible:
diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
index c9c25132d154..d4366a11918e 100644
--- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
+++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
@@ -117,7 +117,9 @@ properties:
type: object
description: CPSWxG NUSS external ports
- $ref: ethernet-controller.yaml#
+ allOf:
+ - $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
unevaluatedProperties: false
properties:
diff --git a/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml b/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml
index 1d33d80af11c..f8894546c02a 100644
--- a/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml
+++ b/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml
@@ -130,7 +130,8 @@ required:
- phy-handle
allOf:
- - $ref: /schemas/net/ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#
+ - $ref: ethernet-controller.yaml#/$defs/phylink
additionalProperties: false
--
2.39.2
The MDIO controlled Realtek switches (distinguished by the reg property
being used) are allowed to have their MDIO bus registered OF-based. Allow
the mdio property for them.
Only the SMI controlled Realtek switches require the compatible property of
the mdio child node. Require it for them and disallow it for the MDIO controlled
ones.
Remove the unnecessary if:then:not:required rule and comments.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
.../devicetree/bindings/net/dsa/realtek.yaml | 20 ++++++++-----------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea..7eb025df0df8 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -109,18 +109,19 @@ if:
then:
$ref: /schemas/spi/spi-peripheral-props.yaml#
- not:
- required:
- - mdc-gpios
- - mdio-gpios
- - mdio
-
properties:
mdc-gpios: false
mdio-gpios: false
- mdio: false
+ mdio:
+ properties:
+ compatible: false
else:
+ properties:
+ mdio:
+ required:
+ - compatible
+
required:
- mdc-gpios
- mdio-gpios
@@ -130,11 +131,6 @@ else:
required:
- compatible
- # - mdc-gpios
- # - mdio-gpios
- # - reset-gpios
- # - mdio
-
unevaluatedProperties: false
examples:
--
2.39.2
Convert the document for Marvell ethernet switches to json-schema.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
.../devicetree/bindings/net/dsa/marvell.txt | 109 ----------
.../devicetree/bindings/net/dsa/marvell.yaml | 204 ++++++++++++++++++
2 files changed, 204 insertions(+), 109 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/net/dsa/marvell.txt
create mode 100644 Documentation/devicetree/bindings/net/dsa/marvell.yaml
diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
deleted file mode 100644
index 6ec0c181b6db..000000000000
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ /dev/null
@@ -1,109 +0,0 @@
-Marvell DSA Switch Device Tree Bindings
----------------------------------------
-
-WARNING: This binding is currently unstable. Do not program it into a
-FLASH never to be changed again. Once this binding is stable, this
-warning will be removed.
-
-If you need a stable binding, use the old dsa.txt binding.
-
-Marvell Switches are MDIO devices. The following properties should be
-placed as a child node of an mdio device.
-
-The properties described here are those specific to Marvell devices.
-Additional required and optional properties can be found in dsa.txt.
-
-The compatibility string is used only to find an identification register,
-which is at a different MDIO base address in different switch families.
-- "marvell,mv88e6085" : Switch has base address 0x10. Use with models:
- 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165,
- 6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321,
- 6341, 6350, 6351, 6352
-- "marvell,mv88e6190" : Switch has base address 0x00. Use with models:
- 6190, 6190X, 6191, 6290, 6361, 6390, 6390X
-- "marvell,mv88e6250" : Switch has base address 0x08 or 0x18. Use with model:
- 6220, 6250
-
-Required properties:
-- compatible : Should be one of "marvell,mv88e6085",
- "marvell,mv88e6190" or "marvell,mv88e6250" as
- indicated above
-- reg : Address on the MII bus for the switch.
-
-Optional properties:
-
-- reset-gpios : Should be a gpio specifier for a reset line
-- interrupts : Interrupt from the switch
-- interrupt-controller : Indicates the switch is itself an interrupt
- controller. This is used for the PHY interrupts.
-#interrupt-cells = <2> : Controller uses two cells, number and flag
-- eeprom-length : Set to the length of an EEPROM connected to the
- switch. Must be set if the switch can not detect
- the presence and/or size of a connected EEPROM,
- otherwise optional.
-- mdio : Container of PHY and devices on the switches MDIO
- bus.
-- mdio? : Container of PHYs and devices on the external MDIO
- bus. The node must contains a compatible string of
- "marvell,mv88e6xxx-mdio-external"
-
-Example:
-
- mdio {
- #address-cells = <1>;
- #size-cells = <0>;
- interrupt-parent = <&gpio0>;
- interrupts = <27 IRQ_TYPE_LEVEL_LOW>;
- interrupt-controller;
- #interrupt-cells = <2>;
-
- switch0: switch@0 {
- compatible = "marvell,mv88e6085";
- reg = <0>;
- reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
-
- mdio {
- #address-cells = <1>;
- #size-cells = <0>;
- switch1phy0: switch1phy0@0 {
- reg = <0>;
- interrupt-parent = <&switch0>;
- interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
- };
- };
- };
- };
-
- mdio {
- #address-cells = <1>;
- #size-cells = <0>;
- interrupt-parent = <&gpio0>;
- interrupts = <27 IRQ_TYPE_LEVEL_LOW>;
- interrupt-controller;
- #interrupt-cells = <2>;
-
- switch0: switch@0 {
- compatible = "marvell,mv88e6190";
- reg = <0>;
- reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
-
- mdio {
- #address-cells = <1>;
- #size-cells = <0>;
- switch1phy0: switch1phy0@0 {
- reg = <0>;
- interrupt-parent = <&switch0>;
- interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
- };
- };
-
- mdio1 {
- compatible = "marvell,mv88e6xxx-mdio-external";
- #address-cells = <1>;
- #size-cells = <0>;
- switch1phy9: switch1phy0@9 {
- reg = <9>;
- };
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.yaml b/Documentation/devicetree/bindings/net/dsa/marvell.yaml
new file mode 100644
index 000000000000..45756b8d74d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.yaml
@@ -0,0 +1,204 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/marvell.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Ethernet Switches
+
+$ref: dsa.yaml#/$defs/ethernet-ports
+
+maintainers:
+ - Andrew Lunn <[email protected]>
+ - Arınç ÜNAL <[email protected]>
+
+description:
+ Marvell ethernet switches are MDIO devices.
+
+properties:
+ compatible:
+ oneOf:
+ - description: |
+ Switch has base address 0x10. Use with models:
+ 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, 6171, 6172, 6175,
+ 6176, 6185, 6240, 6320, 6321, 6341, 6350, 6351, 6352
+ const: marvell,mv88e6085
+
+ - description: |
+ Switch has base address 0x00. Use with models:
+ 6190, 6190X, 6191, 6290, 6361, 6390, 6390X
+ const: marvell,mv88e6190
+
+ - description: |
+ Switch has base address 0x08 or 0x18. Use with models:
+ 6220, 6250
+ const: marvell,mv88e6250
+
+ reg:
+ maxItems: 1
+
+ reset-gpios:
+ description: GPIO to be used to reset the whole device
+ maxItems: 1
+
+ "#interrupt-cells":
+ const: 2
+
+ interrupt-controller: true
+
+ interrupts:
+ maxItems: 1
+
+ eeprom-length:
+ description:
+ Set to the length of an EEPROM connected to the switch. Must be set if the
+ switch can not detect the presence and/or size of a connected EEPROM,
+ otherwise optional.
+
+ mdio:
+ description:
+ The optional node for the MDIO bus of the switch. The bus will be
+ registered non-OF-based if this is not defined.
+ $ref: /schemas/net/mdio.yaml#
+
+ mdio-external:
+ description: The externally reachable MDIO bus of the 6390 family switches
+ $ref: /schemas/net/mdio.yaml#
+
+ properties:
+ compatible:
+ const: marvell,mv88e6xxx-mdio-external
+
+ required:
+ - compatible
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ enum:
+ - marvell,mv88e6085
+ - marvell,mv88e6250
+ then:
+ properties:
+ mdio-external: false
+
+ - if:
+ required: [ mdio ]
+ then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ switch@0 {
+ compatible = "marvell,mv88e6085";
+ reg = <0>;
+ reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
+
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ phy-mode = "rmii";
+ ethernet = <&fec1>;
+
+ fixed-link {
+ speed = <100>;
+ full-duplex;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ label = "swp1";
+ };
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ switch: switch@0 {
+ compatible = "marvell,mv88e6190";
+ reg = <0>;
+ reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
+ interrupt-parent = <&gpio3>;
+ interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ phy-mode = "rmii";
+ ethernet = <&fec1>;
+
+ fixed-link {
+ speed = <100>;
+ full-duplex;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ label = "swp1";
+ phy-handle = <ðphy1>;
+ };
+
+ port@2 {
+ reg = <2>;
+ label = "swp2";
+ phy-handle = <&ext_ethphy0>;
+ };
+ };
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethphy1: ethernet-phy@1 {
+ reg = <1>;
+ interrupt-parent = <&switch>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
+
+ mdio-external {
+ compatible = "marvell,mv88e6xxx-mdio-external";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ext_ethphy0: ethernet-phy@0 {
+ reg = <0>;
+ compatible = "ethernet-phy-ieee802.3-c45";
+ };
+ };
+ };
+ };
--
2.39.2
The SJA1110 switch uses the mdios property for its two MDIO buses. Instead
of a pattern, define two mdio nodes. This ensures the same compatible
string won't be used twice. The address and size cell definitions can also
be removed now that the reg property has become unnecessary.
Move the comment to the description of mdios, mdio0, and mdio1 properties.
Disallow the mdios property for SJA1105. Require at least one of the MDIO
buses to be defined to prevent empty mdios child node.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
.../bindings/net/dsa/nxp,sja1105.yaml | 49 ++++++++++---------
1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 4d5f5cc6d031..3f3c4ecc6442 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -35,42 +35,44 @@ properties:
spi-cpha: true
spi-cpol: true
- # Optional container node for the 2 internal MDIO buses of the SJA1110
- # (one for the internal 100base-T1 PHYs and the other for the single
- # 100base-TX PHY). The "reg" property does not have physical significance.
- # The PHY addresses to port correspondence is as follows: for 100base-T1,
- # port 5 has PHY 1, port 6 has PHY 2 etc, while for 100base-TX, port 1 has
- # PHY 1.
mdios:
+ description:
+ The optional container node for the two MDIO buses of the SJA1110.
type: object
properties:
- '#address-cells':
- const: 1
- '#size-cells':
- const: 0
-
- patternProperties:
- "^mdio@[0-1]$":
+ mdio0:
+ description:
+ The node for the bus controlling the 100base-T1 PHYs of the SJA1110
+ switch. PHY address to port correspondence is port 5 has PHY 1, port 6
+ has PHY 2, etc.
$ref: /schemas/net/mdio.yaml#
unevaluatedProperties: false
properties:
compatible:
- oneOf:
- - enum:
- - nxp,sja1110-base-t1-mdio
- - nxp,sja1110-base-tx-mdio
+ const: nxp,sja1110-base-t1-mdio
+
+ required:
+ - compatible
- reg:
- oneOf:
- - enum:
- - 0
- - 1
+ mdio1:
+ description:
+ The node for the bus controlling the 100base-TX PHY of the SJA1110
+ switch. PHY address to port correspondence is port 1 has PHY 1.
+ $ref: /schemas/net/mdio.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ compatible:
+ const: nxp,sja1110-base-tx-mdio
required:
- compatible
- - reg
+
+ anyOf:
+ - required: [ mdio0 ]
+ - required: [ mdio1 ]
patternProperties:
"^(ethernet-)?ports$":
@@ -124,6 +126,7 @@ allOf:
then:
properties:
spi-cpol: false
+ mdios: false
required:
- spi-cpha
else:
--
2.39.2
On Sat, Sep 16, 2023 at 02:08:59PM +0300, Arınç ÜNAL wrote:
> Phylink bindings are required for ethernet controllers that utilise
> phylink_fwnode_phy_connect() directly or through phylink_of_phy_connect(),
> and register OF-based only MDIO buses, if they register any.
What is phylink?
Don't describe/justify binding changes based on some Linux functions.
> All the drivers that utilise phylink_fwnode_phy_connect() directly or
> through phylink_of_phy_connect():
>
> - DSA
> - drivers/net/ethernet/mscc/ocelot_net.c
> - mscc,vsc7514-switch.yaml
> - drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> - microchip,sparx5-switch.yaml
> - drivers/net/ethernet/altera/altera_tse_main.c
> - altr,tse.yaml
> - drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> - xlnx,axi-ethernet.yaml
> - drivers/net/ethernet/mediatek/mtk_eth_soc.c
> - mediatek,net.yaml
> - drivers/net/ethernet/ti/am65-cpsw-nuss.c
> - ti,k3-am654-cpsw-nuss.yaml
> - drivers/net/ethernet/atheros/ag71xx.c
> - qca,ar71xx.yaml
> - drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> - fsl,fman-dtsec.yaml
> - drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> - microchip,lan966x-switch.yaml
> - drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> - marvell,pp2.yaml
> - drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> - fsl,qoriq-mc-dpmac.yaml
> - drivers/net/ethernet/cadence/macb_main.c
> - cdns,macb.yaml
> - Can register non-OF-based bus.
> - drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> - snps,dwmac.yaml
> - Can register non-OF-based bus.
> - drivers/net/ethernet/marvell/mvneta.c
> - marvell-armada-370-neta.txt
> - drivers/net/ethernet/freescale/enetc/enetc.c
> - fsl-enetc.txt
>
> RFC: The drivers marked with "can register non-OF-based bus" seem to search
> the MDIO bus to connect the PHY to the MAC using phylink_connect_phy()
> and/or phy_find_first() if phylink bindings don't exist. Should we enforce
> phylink bindings on their schemas regardless?
Generally, describing the MDIO bus in DT is optional because the devices
on the bus can be discovered. But then sometimes a device can't be
discovered or has additional properties which aren't discoverable. So in
general, an MDIO bus in DT should always be optional, but always
supported (and validated) if present. If the device has a separate node
for the MDIO controller (i.e. one with a compatible and reg for the MDIO
controller register), then that should always be there (because the h/w
is always there).
Rob
On Sat, Sep 16, 2023 at 02:09:00PM +0300, Arınç ÜNAL wrote:
> Convert the document for Marvell ethernet switches to json-schema.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/marvell.txt | 109 ----------
> .../devicetree/bindings/net/dsa/marvell.yaml | 204 ++++++++++++++++++
> 2 files changed, 204 insertions(+), 109 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/net/dsa/marvell.txt
> create mode 100644 Documentation/devicetree/bindings/net/dsa/marvell.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> deleted file mode 100644
> index 6ec0c181b6db..000000000000
> --- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
> +++ /dev/null
> @@ -1,109 +0,0 @@
> -Marvell DSA Switch Device Tree Bindings
> ----------------------------------------
> -
> -WARNING: This binding is currently unstable. Do not program it into a
> -FLASH never to be changed again. Once this binding is stable, this
> -warning will be removed.
> -
> -If you need a stable binding, use the old dsa.txt binding.
> -
> -Marvell Switches are MDIO devices. The following properties should be
> -placed as a child node of an mdio device.
> -
> -The properties described here are those specific to Marvell devices.
> -Additional required and optional properties can be found in dsa.txt.
> -
> -The compatibility string is used only to find an identification register,
> -which is at a different MDIO base address in different switch families.
> -- "marvell,mv88e6085" : Switch has base address 0x10. Use with models:
> - 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165,
> - 6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321,
> - 6341, 6350, 6351, 6352
> -- "marvell,mv88e6190" : Switch has base address 0x00. Use with models:
> - 6190, 6190X, 6191, 6290, 6361, 6390, 6390X
> -- "marvell,mv88e6250" : Switch has base address 0x08 or 0x18. Use with model:
> - 6220, 6250
> -
> -Required properties:
> -- compatible : Should be one of "marvell,mv88e6085",
> - "marvell,mv88e6190" or "marvell,mv88e6250" as
> - indicated above
> -- reg : Address on the MII bus for the switch.
> -
> -Optional properties:
> -
> -- reset-gpios : Should be a gpio specifier for a reset line
> -- interrupts : Interrupt from the switch
> -- interrupt-controller : Indicates the switch is itself an interrupt
> - controller. This is used for the PHY interrupts.
> -#interrupt-cells = <2> : Controller uses two cells, number and flag
> -- eeprom-length : Set to the length of an EEPROM connected to the
> - switch. Must be set if the switch can not detect
> - the presence and/or size of a connected EEPROM,
> - otherwise optional.
> -- mdio : Container of PHY and devices on the switches MDIO
> - bus.
> -- mdio? : Container of PHYs and devices on the external MDIO
> - bus. The node must contains a compatible string of
> - "marvell,mv88e6xxx-mdio-external"
> -
> -Example:
> -
> - mdio {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - interrupt-parent = <&gpio0>;
> - interrupts = <27 IRQ_TYPE_LEVEL_LOW>;
> - interrupt-controller;
> - #interrupt-cells = <2>;
> -
> - switch0: switch@0 {
> - compatible = "marvell,mv88e6085";
> - reg = <0>;
> - reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
> -
> - mdio {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - switch1phy0: switch1phy0@0 {
> - reg = <0>;
> - interrupt-parent = <&switch0>;
> - interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> - };
> - };
> - };
> - };
> -
> - mdio {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - interrupt-parent = <&gpio0>;
> - interrupts = <27 IRQ_TYPE_LEVEL_LOW>;
> - interrupt-controller;
> - #interrupt-cells = <2>;
> -
> - switch0: switch@0 {
> - compatible = "marvell,mv88e6190";
> - reg = <0>;
> - reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
> -
> - mdio {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - switch1phy0: switch1phy0@0 {
> - reg = <0>;
> - interrupt-parent = <&switch0>;
> - interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> - };
> - };
> -
> - mdio1 {
> - compatible = "marvell,mv88e6xxx-mdio-external";
> - #address-cells = <1>;
> - #size-cells = <0>;
> - switch1phy9: switch1phy0@9 {
> - reg = <9>;
> - };
> - };
> - };
> - };
> diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.yaml b/Documentation/devicetree/bindings/net/dsa/marvell.yaml
> new file mode 100644
> index 000000000000..45756b8d74d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/marvell.yaml
> @@ -0,0 +1,204 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/marvell.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Ethernet Switches
> +
> +$ref: dsa.yaml#/$defs/ethernet-ports
> +
> +maintainers:
> + - Andrew Lunn <[email protected]>
> + - Arınç ÜNAL <[email protected]>
> +
> +description:
> + Marvell ethernet switches are MDIO devices.
> +
> +properties:
> + compatible:
> + oneOf:
> + - description: |
> + Switch has base address 0x10. Use with models:
> + 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, 6171, 6172, 6175,
> + 6176, 6185, 6240, 6320, 6321, 6341, 6350, 6351, 6352
> + const: marvell,mv88e6085
> +
> + - description: |
> + Switch has base address 0x00. Use with models:
> + 6190, 6190X, 6191, 6290, 6361, 6390, 6390X
> + const: marvell,mv88e6190
> +
> + - description: |
> + Switch has base address 0x08 or 0x18. Use with models:
> + 6220, 6250
> + const: marvell,mv88e6250
> +
> + reg:
> + maxItems: 1
> +
> + reset-gpios:
> + description: GPIO to be used to reset the whole device
> + maxItems: 1
> +
> + "#interrupt-cells":
> + const: 2
> +
> + interrupt-controller: true
> +
> + interrupts:
> + maxItems: 1
> +
> + eeprom-length:
> + description:
> + Set to the length of an EEPROM connected to the switch. Must be set if the
> + switch can not detect the presence and/or size of a connected EEPROM,
> + otherwise optional.
> +
> + mdio:
> + description:
> + The optional node for the MDIO bus of the switch. The bus will be
> + registered non-OF-based if this is not defined.
That's a detail of the OS behavior.
> + $ref: /schemas/net/mdio.yaml#
unevaluatedProperties: false
> +
> + mdio-external:
> + description: The externally reachable MDIO bus of the 6390 family switches
> + $ref: /schemas/net/mdio.yaml#
unevaluatedProperties: false
> +
> + properties:
> + compatible:
> + const: marvell,mv88e6xxx-mdio-external
> +
> + required:
> + - compatible
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + enum:
> + - marvell,mv88e6085
> + - marvell,mv88e6250
> + then:
> + properties:
> + mdio-external: false
> +
> + - if:
> + required: [ mdio ]
> + then:
> + patternProperties:
> + "^(ethernet-)?ports$":
> + patternProperties:
> + "^(ethernet-)?port@[0-9]+$":
> + $ref: /schemas/net/ethernet-controller.yaml#/$defs/phylink
This probably doesn't work right. The problem is every node ultimately
needs a single schema with all possible properties in order to not allow
undefined properties. This is the reason for the complexity with a $ref
at each level in schemas for these nodes.
I'm not sure it is worth the complexity to enforce what you want here.
It may be easier to just always allow phylink properties in
ethernet-port nodes.
As this is a switch, it should be referencing ethernet-switch.yaml.
Rob
On Sat, Sep 16, 2023 at 02:08:56PM +0300, Arınç ÜNAL wrote:
> The SJA1110 switch uses the mdios property for its two MDIO buses. Instead
> of a pattern, define two mdio nodes. This ensures the same compatible
> string won't be used twice. The address and size cell definitions can also
> be removed now that the reg property has become unnecessary.
>
> Move the comment to the description of mdios, mdio0, and mdio1 properties.
> Disallow the mdios property for SJA1105. Require at least one of the MDIO
> buses to be defined to prevent empty mdios child node.
It's an ABI. You can't just change this.
You can split the pattern into 'mdio@0' and 'mdio@1' with different
compatibles for each if you want.
Rob
> - Link descriptions must be required on ethernet controllers. We don't care
> whether some Linux driver can or cannot find the PHY or set up a fixed
> link without looking at the devicetree.
That can lead to future surprises, and breakage.
Something which is not used is not tested, and so sometimes wrong, and
nobody knows. Say the driver is extended to a new device and actually
does need to use this never before used information. You then find it
is wrong, and you get a regression.
We have had issues like this before. There are four rgmii phy-link
modes. We have had PHY drivers which ignored one of those modes, it
silently accepted it, but did not change the hardware to actually use
that mode. The PHY continues to use its reset defaults or strapping,
and it worked. A lot of device trees ended up using this mode. And it
was not the same as reset defaults/strapping.
And then somebody needed that fourth mode, and made it actually
work. And all those boards wrongly using that mode broke.
The lesson i learned from that episode is that anything in device tree
must actually be used and tested.
> Although I see dsa.yaml and dsa-port.yaml mostly consist of describing an
> ethernet switch with CPU port(s), there're properties that are specific to
> DSA, such as dsa,member on dsa.yaml and dsa-tag-protocol and label on
> dsa-port.yaml.
I would say dsa,member does describe the hardware. It provides two
bits of information:
Which cluster of switches does this switch belong to. You probably can
derive it using the DSA links between switches, which is also a
hardware property. But having it in device tree makes it simpler.
Which switch is this within a cluster. You need to be able to say:
Send this frame out Port X of switch Y. How does a switch know it is
Y? It could be strapping, which is clearly a hardware property.
dsa-tag-protocol is similar to phy-mode. It tells you the protocol
running between the CPU port and the SoC master interface. You often
can imply it, but again, it could be determined by strapping on the
switch.
label is an interesting one, and probably would not be accepted if it
was proposed now. But it has been around a long time. It also does
describe the hardware, it is what is printed on the case next to the
RJ45. To make the user experience simpler, we then try to make the
linux interface name match the label on the case.
Andrew
On 21.09.2023 16:00, Andrew Lunn wrote:
>> - Link descriptions must be required on ethernet controllers. We don't care
>> whether some Linux driver can or cannot find the PHY or set up a fixed
>> link without looking at the devicetree.
>
> That can lead to future surprises, and breakage.
>
> Something which is not used is not tested, and so sometimes wrong, and
> nobody knows. Say the driver is extended to a new device and actually
> does need to use this never before used information. You then find it
> is wrong, and you get a regression.
>
> We have had issues like this before. There are four rgmii phy-link
> modes. We have had PHY drivers which ignored one of those modes, it
> silently accepted it, but did not change the hardware to actually use
> that mode. The PHY continues to use its reset defaults or strapping,
> and it worked. A lot of device trees ended up using this mode. And it
> was not the same as reset defaults/strapping.
>
> And then somebody needed that fourth mode, and made it actually
> work. And all those boards wrongly using that mode broke.
>
> The lesson i learned from that episode is that anything in device tree
> must actually be used and tested.
It looks like the root cause here was the lack of dt-bindings to only allow
the phy-mode values the hardware supports. What I see here is the driver
change should've been tested on all different hardware the driver controls
then the improper describing of hardware on the devicetree source file
addressed.
If a devicetree change that ensures proper describing of hardware is found
to break a driver in the future, then that exposes a bug on the driver and
the driver will have to be fixed. I don't see this upholding writing
dt-bindings that ensures proper describing of the hardware.
>
>> Although I see dsa.yaml and dsa-port.yaml mostly consist of describing an
>> ethernet switch with CPU port(s), there're properties that are specific to
>> DSA, such as dsa,member on dsa.yaml and dsa-tag-protocol and label on
>> dsa-port.yaml.
>
> I would say dsa,member does describe the hardware. It provides two
> bits of information:
>
> Which cluster of switches does this switch belong to. You probably can
> derive it using the DSA links between switches, which is also a
> hardware property. But having it in device tree makes it simpler.
>
> Which switch is this within a cluster. You need to be able to say:
> Send this frame out Port X of switch Y. How does a switch know it is
> Y? It could be strapping, which is clearly a hardware property.
>
> dsa-tag-protocol is similar to phy-mode. It tells you the protocol
> running between the CPU port and the SoC master interface. You often
> can imply it, but again, it could be determined by strapping on the
> switch.
>
> label is an interesting one, and probably would not be accepted if it
> was proposed now. But it has been around a long time. It also does
> describe the hardware, it is what is printed on the case next to the
> RJ45. To make the user experience simpler, we then try to make the
> linux interface name match the label on the case.
Looks like we can incorporate dsa.port and dsa-port.yaml into
ethernet-switch.yaml and ethernet-switch-port.yaml with adjustments.
Arınç
On Thu, Sep 21, 2023 at 09:21:40PM +0300, Arınç ÜNAL wrote:
> On 21.09.2023 16:00, Andrew Lunn wrote:
> > > - Link descriptions must be required on ethernet controllers. We don't care
> > > whether some Linux driver can or cannot find the PHY or set up a fixed
> > > link without looking at the devicetree.
> >
> > That can lead to future surprises, and breakage.
> >
> > Something which is not used is not tested, and so sometimes wrong, and
> > nobody knows. Say the driver is extended to a new device and actually
> > does need to use this never before used information. You then find it
> > is wrong, and you get a regression.
> >
> > We have had issues like this before. There are four rgmii phy-link
> > modes. We have had PHY drivers which ignored one of those modes, it
> > silently accepted it, but did not change the hardware to actually use
> > that mode. The PHY continues to use its reset defaults or strapping,
> > and it worked. A lot of device trees ended up using this mode. And it
> > was not the same as reset defaults/strapping.
> >
> > And then somebody needed that fourth mode, and made it actually
> > work. And all those boards wrongly using that mode broke.
> >
> > The lesson i learned from that episode is that anything in device tree
> > must actually be used and tested.
>
> It looks like the root cause here was the lack of dt-bindings to
> only allow the phy-mode values the hardware supports.
That would not help. The hardware supported all 4 RGMII modes. So
listing all four in the dt-binding would be correct. But the driver
for the hardware had a bug, and so silently ignored one of the
modes. That then masked the bugs in board DT files.
> What I see here is the driver change should've been tested on all
> different hardware the driver controls then the improper describing
> of hardware on the devicetree source file addressed.
Which is what did happen. But it took a while to find all those broken
boards. For a period of time, we had regressions.
Bugs happen. It is a fact of life. But we want those bugs to be easy
to find as possible. If we force DT writers to add properties which
the driver never uses, they are going to be bugs in those
properties. And those bugs are not going to be easy to find, and quite
likely, they will only be found a long time after they are added. We
should not be adding unused properties and bugs just to keep a yaml
checker happy.
Andrew
On 18.09.2023 21:13, Rob Herring wrote:
> On Sat, Sep 16, 2023 at 02:08:59PM +0300, Arınç ÜNAL wrote:
>> Phylink bindings are required for ethernet controllers that utilise
>> phylink_fwnode_phy_connect() directly or through phylink_of_phy_connect(),
>> and register OF-based only MDIO buses, if they register any.
>
> What is phylink?
>
> Don't describe/justify binding changes based on some Linux functions.
I'd like to discuss the influence of the Linux drivers on the devicetree
bindings. Before that let me clarify these "phylink" bindings. They are
certain rules to properly describe the link of the ethernet controller. Any
of the phy-handle, pcs-handle, sfp, and fixed-link properties describe the
link. They are already defined on ethernet-controller.yaml, just not
enforced.
Why I called them phylink bindings, is because when phylink is used on the
ethernet controller driver, only then we can be sure that the link
descriptions on the ethernet controller are needed. Because, some drivers
don't need the link descriptions on the ethernet controller as they can
search a certain MDIO bus to find and connect a MAC to a PHY or set up a
fixed link without looking at the devicetree.
I don't like this approach because, as you said, devicetree bindings should
describe the hardware, not driver policies. So I propose that we don't let
Linux drivers interfere with dt-bindings. To that extent:
- Link descriptions must be required on ethernet controllers. We don't care
whether some Linux driver can or cannot find the PHY or set up a fixed
link without looking at the devicetree.
- The MDIO bus description on an ethernet controller describes that the
hardware comes with an MDIO bus. The MDIO property is optional not
because some Linux driver can find and map the PHYs to MACs without DT
descriptions but because the hardware doesn't have to use that or any
MDIO bus.
Let's also talk about the elephant in the room: DSA bindings. Distributed
Switch Architecture is a Linux subsystem, not a hardware property.
Devicetree bindings are supposed to describe the hardware, yet we have
bindings specifically for DSA so that the DSA driver can properly control
the hardware.
Although I see dsa.yaml and dsa-port.yaml mostly consist of describing an
ethernet switch with CPU port(s), there're properties that are specific to
DSA, such as dsa,member on dsa.yaml and dsa-tag-protocol and label on
dsa-port.yaml.
Depending on how I get responses, I will address my patches. Then I can
further discuss the DSA bindings.
Arınç
On Sat, Sep 16, 2023 at 02:08:52PM +0300, Arınç ÜNAL wrote:
> Hello there.
>
> This patch series defines phylink bindings and enforces them for the
> ethernet controllers that need them.
>
> Some schemas had to be changed to properly enforce phylink bindings for all
> of the affected ethernet controllers. Some of the documents of these
> ethernet controllers were non json-schema, which had to be converted.
>
> I will convert the remaining documents to json-schema while this patch
> series receives reviews.
I can't say that I'm comfortable with this. We appear to be defining
bindings based on software implementation, and a desire for the DT
tooling to enforce what the software implementation wants. Isn't this
against the aims of device tree and device tree binding documentation?
Seems to me like feature-creep.
The bindings that phylink parses are already documented in the
ethernet controller yaml document. Specifically:
- phylink does not parse the phy-mode property, that is left to the
implementation to pass to phylink, which can implement it any
which way they choose (and even default to something.)
- phylink does not require a phy property - phylink does expect a PHY
to be attached, but how that PHY is attached is up to the ethernet
controller driver. It may call one of the phylink functions that
parses the phy property, or it may manually supply the phy device to
phylink. Either way, phylink does not itself require a PHY property.
- phylink does not require a sfp property - this obviously is optional.
So, all in all, ethernet-controller already describes it, and to create
a DT binding document that pretends that phylink requires any of this
stuff is, in my mind, wrong.
DSA requires certain properties by dint of the parsing and setup of
phylink being in generic code - this is not because phylink requires
certain properties, but phylink does require certain information in
order to function correctly.
The issue here is _how_ phylink gets that information, and as I state
above, it _can_ come from DT, but it can also be given that information
manually.
As an example, there are plenty of drivers in the tree which try to
parse a phy node, and if that's not present, they try to see if a PHY
exists at a default# bus address.
We seem to be digging outselves a hole here, where "phylink must have
these properties". No, that is wrong.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Sep 23, 2023 at 12:57:52AM +0300, Arınç ÜNAL wrote:
> On 22/09/2023 15:40, Russell King (Oracle) wrote:
> > On Sat, Sep 16, 2023 at 02:08:52PM +0300, Arınç ÜNAL wrote:
> > > Hello there.
> > >
> > > This patch series defines phylink bindings and enforces them for the
> > > ethernet controllers that need them.
> > >
> > > Some schemas had to be changed to properly enforce phylink bindings for all
> > > of the affected ethernet controllers. Some of the documents of these
> > > ethernet controllers were non json-schema, which had to be converted.
> > >
> > > I will convert the remaining documents to json-schema while this patch
> > > series receives reviews.
> >
> > I can't say that I'm comfortable with this. We appear to be defining
> > bindings based on software implementation, and a desire for the DT
> > tooling to enforce what the software implementation wants. Isn't this
> > against the aims of device tree and device tree binding documentation?
> > Seems to me like feature-creep.
> >
> > The bindings that phylink parses are already documented in the
> > ethernet controller yaml document. Specifically:
> >
> > - phylink does not parse the phy-mode property, that is left to the
> > implementation to pass to phylink, which can implement it any
> > which way they choose (and even default to something.)
> >
> > - phylink does not require a phy property - phylink does expect a PHY
> > to be attached, but how that PHY is attached is up to the ethernet
> > controller driver. It may call one of the phylink functions that
> > parses the phy property, or it may manually supply the phy device to
> > phylink. Either way, phylink does not itself require a PHY property.
> >
> > - phylink does not require a sfp property - this obviously is optional.
> >
> > So, all in all, ethernet-controller already describes it, and to create
> > a DT binding document that pretends that phylink requires any of this
> > stuff is, in my mind, wrong.
> >
> > DSA requires certain properties by dint of the parsing and setup of
> > phylink being in generic code - this is not because phylink requires
> > certain properties, but phylink does require certain information in
> > order to function correctly.
> >
> > The issue here is _how_ phylink gets that information, and as I state
> > above, it _can_ come from DT, but it can also be given that information
> > manually.
> >
> > As an example, there are plenty of drivers in the tree which try to
> > parse a phy node, and if that's not present, they try to see if a PHY
> > exists at a default# bus address.
> >
> > We seem to be digging outselves a hole here, where "phylink must have
> > these properties". No, that is wrong.
>
> I agree. My patch description here failed to explain the actual issue,
> which is missing hardware descriptions. Here's what I understand. An
> ethernet-controller is a MAC. For the MAC to work properly with its link
> partner, at least one of these must be described:
> - pointer to a PHY to retrieve link information from the PHY
> - pointer to a PCS to retrieve link information from the PCS
> - pointer to an SFP to retrieve link information from the SFP
> - static link information
What about something like macb? The macb driver:
- attempts to connect a phy using phylink_of_phy_connect()
- if that fails, and there is no phy-handle property, then the driver
will attempt to find the first PHY to exist on its MII bus, and will
connect that using phylink_connect_phy().
So, in this case, if we define a phylink binding to require one of a
phy-handle node, pcs node, sfp node or static link information, then
although macb uses phylink, it then doesn't conform to this phylink
binding. (This is the only driver that uses phy_find_first() which
also uses phylink according to my greps, but I haven't checked for
any other games drivers be using.)
The same thing more or less happens with non-phylink drivers. Take a
look at drivers/net/ethernet/microchip/lan743x_main.c, and notice
that it first attempts to get a PHY from DT. If that fails, it
uses phy_find_first(). If that fails, and we have a LAN7431, then
a gigabit full-duplex fixed-link PHY is used instead. So, what macb
is doing with phylink is no different from what other drivers are
doing with phylib - and it's the driver's choice.
The same way that there are multiple drivers that don't do this,
which want a PHY device to be specified in DT if the driver was
bound to a device that was described in DT - there are phylink
and non-phylink drivers that do this.
This is exactly my point - there is *no* *such* *thing* as a phylink
binding. There is the ethernet-controller binding, which phylink
provides the ability for network drivers to optionally use, but
phylink doesn't require anything from any firmware description, except
to attach a SFP interface, or to describe a fixed-link. Everything else
is really up to the ethernet-controller aka MAC driver to decide how it
wants to deal with things.
We currently work around this by the ethernet-controller YAML having
all these properties as optional. Maybe some drivers extend that YAML
and require certain properties - that is their perogative, but that is
the driver's choice, and is a completely separate issue to whether
the driver is using phylink or not.
The real question is how do we want to describe an ethernet controller
and what properties should we be requiring for it (if any). Maybe if we
want to require one of a PHY, PCS, SFP, or fixed-link, maybe we should
have that as a strictly-checked ethernet controller which drivers can
opt into using if that's what they require.
However, to dress this up as "phylink requires xyz, so lets create
a phylink binding description" is just wrong.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> I agree. My patch description here failed to explain the actual issue,
> which is missing hardware descriptions. Here's what I understand. An
> ethernet-controller is a MAC. For the MAC to work properly with its link
> partner, at least one of these must be described:
> - pointer to a PHY to retrieve link information from the PHY
> - pointer to a PCS to retrieve link information from the PCS
> - pointer to an SFP to retrieve link information from the SFP
> - static link information
You are missing:
- The MAC has firmware driving the PHY, nothing for linux to do.
There are properties in ethernet-controller.yaml the MAC driver would
however like to use such as local-mac-address, max-frame-size,
nvmem-cell-names etc.
Andrew
On 22/09/2023 15:40, Russell King (Oracle) wrote:
> On Sat, Sep 16, 2023 at 02:08:52PM +0300, Arınç ÜNAL wrote:
>> Hello there.
>>
>> This patch series defines phylink bindings and enforces them for the
>> ethernet controllers that need them.
>>
>> Some schemas had to be changed to properly enforce phylink bindings for all
>> of the affected ethernet controllers. Some of the documents of these
>> ethernet controllers were non json-schema, which had to be converted.
>>
>> I will convert the remaining documents to json-schema while this patch
>> series receives reviews.
>
> I can't say that I'm comfortable with this. We appear to be defining
> bindings based on software implementation, and a desire for the DT
> tooling to enforce what the software implementation wants. Isn't this
> against the aims of device tree and device tree binding documentation?
> Seems to me like feature-creep.
>
> The bindings that phylink parses are already documented in the
> ethernet controller yaml document. Specifically:
>
> - phylink does not parse the phy-mode property, that is left to the
> implementation to pass to phylink, which can implement it any
> which way they choose (and even default to something.)
>
> - phylink does not require a phy property - phylink does expect a PHY
> to be attached, but how that PHY is attached is up to the ethernet
> controller driver. It may call one of the phylink functions that
> parses the phy property, or it may manually supply the phy device to
> phylink. Either way, phylink does not itself require a PHY property.
>
> - phylink does not require a sfp property - this obviously is optional.
>
> So, all in all, ethernet-controller already describes it, and to create
> a DT binding document that pretends that phylink requires any of this
> stuff is, in my mind, wrong.
>
> DSA requires certain properties by dint of the parsing and setup of
> phylink being in generic code - this is not because phylink requires
> certain properties, but phylink does require certain information in
> order to function correctly.
>
> The issue here is _how_ phylink gets that information, and as I state
> above, it _can_ come from DT, but it can also be given that information
> manually.
>
> As an example, there are plenty of drivers in the tree which try to
> parse a phy node, and if that's not present, they try to see if a PHY
> exists at a default# bus address.
>
> We seem to be digging outselves a hole here, where "phylink must have
> these properties". No, that is wrong.
I agree. My patch description here failed to explain the actual issue,
which is missing hardware descriptions. Here's what I understand. An
ethernet-controller is a MAC. For the MAC to work properly with its link
partner, at least one of these must be described:
- pointer to a PHY to retrieve link information from the PHY
- pointer to a PCS to retrieve link information from the PCS
- pointer to an SFP to retrieve link information from the SFP
- static link information
Andrew under the discussion of patch 7 said that enforcing this may expose
bugs on MAC drivers that never looked at the devicetree to control the
MAC's link which would cause regressions, implying we should hold back on
enforcing it. I've agreed not to enforce it, not because it is incorrect
description of ethernet controller hardware - I think it is correct - but
because I won't be the one to deal with the regressions when this
dt-bindings change goes through.
I won't also enforce it selectively, as saying "these drivers use
phylink_fwnode_phy_connect() therefore there won't be any bad surprises on
the hardware they control so let's enforce it only for them" is nonsense in
the context of describing hardware.
I will focus on documenting the missing MDIO bus descriptions on certain
ethernet switches and converting ethernet switch documents (maybe ethernet
controllers too) to json-schema. There's the incorrect link descriptions on
dsa-port.yaml as confirmed by Vladimir on the discussion of v1 series so
I'll fix that.
I've also got some ethernet controller rules that I think won't break any
driver so I will submit them as well.
Arınç
> However, to dress this up as "phylink requires xyz, so lets create
> a phylink binding description" is just wrong.
+1
Also, phylink is a Linux implementation detail. Other OSes using the
binding don't need to have phylink. Yet they can still use the DT
blobs because they should describe the hardware, independent of how
the OS drives that hardware.
Andrew
On 22.09.2023 02:29, Andrew Lunn wrote:
> On Thu, Sep 21, 2023 at 09:21:40PM +0300, Arınç ÜNAL wrote:
>> On 21.09.2023 16:00, Andrew Lunn wrote:
>>>> - Link descriptions must be required on ethernet controllers. We don't care
>>>> whether some Linux driver can or cannot find the PHY or set up a fixed
>>>> link without looking at the devicetree.
>>>
>>> That can lead to future surprises, and breakage.
>>>
>>> Something which is not used is not tested, and so sometimes wrong, and
>>> nobody knows. Say the driver is extended to a new device and actually
>>> does need to use this never before used information. You then find it
>>> is wrong, and you get a regression.
>>>
>>> We have had issues like this before. There are four rgmii phy-link
>>> modes. We have had PHY drivers which ignored one of those modes, it
>>> silently accepted it, but did not change the hardware to actually use
>>> that mode. The PHY continues to use its reset defaults or strapping,
>>> and it worked. A lot of device trees ended up using this mode. And it
>>> was not the same as reset defaults/strapping.
>>>
>>> And then somebody needed that fourth mode, and made it actually
>>> work. And all those boards wrongly using that mode broke.
>>>
>>> The lesson i learned from that episode is that anything in device tree
>>> must actually be used and tested.
>>
>> It looks like the root cause here was the lack of dt-bindings to
>> only allow the phy-mode values the hardware supports.
>
> That would not help. The hardware supported all 4 RGMII modes. So
> listing all four in the dt-binding would be correct. But the driver
> for the hardware had a bug, and so silently ignored one of the
> modes. That then masked the bugs in board DT files.
>
>> What I see here is the driver change should've been tested on all
>> different hardware the driver controls then the improper describing
>> of hardware on the devicetree source file addressed.
>
> Which is what did happen. But it took a while to find all those broken
> boards. For a period of time, we had regressions.
>
> Bugs happen. It is a fact of life. But we want those bugs to be easy
> to find as possible. If we force DT writers to add properties which
> the driver never uses, they are going to be bugs in those
> properties. And those bugs are not going to be easy to find, and quite
> likely, they will only be found a long time after they are added. We
> should not be adding unused properties and bugs just to keep a yaml
> checker happy.
Understood. Then I will refrain from enforcing link descriptions altogether
as I can't justify enforcing them on ethernet controllers selectively
because certain Linux drivers don't make use of the link properties and may
expose bugs if used.
The link properties are still allowed for all ethernet controllers though,
just not enforced. But I suppose it's better to optionally find the bug
instead of causing a regression then finding the bug.
Arınç
On 23.09.2023 01:36, Andrew Lunn wrote:
>> I agree. My patch description here failed to explain the actual issue,
>> which is missing hardware descriptions. Here's what I understand. An
>> ethernet-controller is a MAC. For the MAC to work properly with its link
>> partner, at least one of these must be described:
>> - pointer to a PHY to retrieve link information from the PHY
>> - pointer to a PCS to retrieve link information from the PCS
>> - pointer to an SFP to retrieve link information from the SFP
>> - static link information
>
> You are missing:
>
> - The MAC has firmware driving the PHY, nothing for linux to do.
>
> There are properties in ethernet-controller.yaml the MAC driver would
> however like to use such as local-mac-address, max-frame-size,
> nvmem-cell-names etc.
This is interesting. This is clearly a hardware difference of the ethernet
controller.
I believe this fits case 1. There's still an MDIO bus the ethernet
controller uses, there's still a PHY on the MDIO bus which the ethernet
controller uses. The only difference is the firmware of the ethernet
controller controls... What exactly does the firmware control that a Linux
driver would have controlled instead? Just configuring the link settings of
the MAC?
If it's just MAC link settings, I believe it would make sense to add a
property on the ethernet controller dt-bindings to state that the hardware
controls the MAC link settings on its own. This way, we would still
describe the MDIO bus and PHY of the ethernet controller while also
pointing out that the MAC link settings are not up to a driver to control.
Arınç
On 23.09.2023 01:29, Russell King (Oracle) wrote:
> On Sat, Sep 23, 2023 at 12:57:52AM +0300, Arınç ÜNAL wrote:
>> I agree. My patch description here failed to explain the actual issue,
>> which is missing hardware descriptions. Here's what I understand. An
>> ethernet-controller is a MAC. For the MAC to work properly with its link
>> partner, at least one of these must be described:
>> - pointer to a PHY to retrieve link information from the PHY
>> - pointer to a PCS to retrieve link information from the PCS
>> - pointer to an SFP to retrieve link information from the SFP
>> - static link information
>
> What about something like macb? The macb driver:
> - attempts to connect a phy using phylink_of_phy_connect()
> - if that fails, and there is no phy-handle property, then the driver
> will attempt to find the first PHY to exist on its MII bus, and will
> connect that using phylink_connect_phy().
>
> So, in this case, if we define a phylink binding to require one of a
> phy-handle node, pcs node, sfp node or static link information, then
> although macb uses phylink, it then doesn't conform to this phylink
> binding. (This is the only driver that uses phy_find_first() which
> also uses phylink according to my greps, but I haven't checked for
> any other games drivers be using.)
>
> The same thing more or less happens with non-phylink drivers. Take a
> look at drivers/net/ethernet/microchip/lan743x_main.c, and notice
> that it first attempts to get a PHY from DT. If that fails, it
> uses phy_find_first(). If that fails, and we have a LAN7431, then
> a gigabit full-duplex fixed-link PHY is used instead. So, what macb
> is doing with phylink is no different from what other drivers are
> doing with phylib - and it's the driver's choice.
>
> The same way that there are multiple drivers that don't do this,
> which want a PHY device to be specified in DT if the driver was
> bound to a device that was described in DT - there are phylink
> and non-phylink drivers that do this.
>
> This is exactly my point - there is *no* *such* *thing* as a phylink
> binding. There is the ethernet-controller binding, which phylink
> provides the ability for network drivers to optionally use, but
> phylink doesn't require anything from any firmware description, except
> to attach a SFP interface, or to describe a fixed-link. Everything else
> is really up to the ethernet-controller aka MAC driver to decide how it
> wants to deal with things.
>
> We currently work around this by the ethernet-controller YAML having
> all these properties as optional. Maybe some drivers extend that YAML
> and require certain properties - that is their perogative, but that is
> the driver's choice, and is a completely separate issue to whether
> the driver is using phylink or not.
>
> The real question is how do we want to describe an ethernet controller
> and what properties should we be requiring for it (if any). Maybe if we
> want to require one of a PHY, PCS, SFP, or fixed-link, maybe we should
> have that as a strictly-checked ethernet controller which drivers can
> opt into using if that's what they require.
I'd like to make this clear. We're only talking about deviating from proper
devicetree bindings so that it won't cause too much work or not at all to
fix the incorrect Linux driver policies.
As long as we don't collectively agree on fixing the drivers to work with
proper devicetree bindings, I'd keep the missing ethernet controller
bindings (requiring at least one of PHY, PCS, SFP, fixed-link) as they
currently are on ethernet-controller.yaml, optional. Or rather, I wouldn't
touch anything regarding this as it's nonsensical to change devicetree
bindings because of driver policies.
As you have pointed out with certain examples, once the driver starts
operating out of what the devicetree says, in other words, once the driver
starts guessing the hardware, there's no guarantee it will always guess it
correctly. As seen with the macb driver, the driver assumes that if there's
no phy-handle property, the PHY on its MDIO bus must be used regardless.
But the MAC may be connected to another MAC, PCS or SFP, meaning it doesn't
use the PHY on that bus.
There is also a case for DSA. If there's an implication that the DSA
controlled switch has an MDIO bus (phy_read() and phy_write()), the DSA
driver will connect the switch MACs to the PHYs on the MDIO bus of the
switch, even if there's no description of that MDIO bus on the devicetree.
As unlikely as it is on a real life scenario, there may be a device that
has its switch MACs wired to the PHYs on another MDIO bus.
This is why I've proposed to make the drivers strictly follow what the
devicetree says.
>
> However, to dress this up as "phylink requires xyz, so lets create
> a phylink binding description" is just wrong.
Agreed.
Arınç
> As you have pointed out with certain examples, once the driver starts
> operating out of what the devicetree says, in other words, once the driver
> starts guessing the hardware, there's no guarantee it will always guess it
> correctly.
This is partially a result of history. Some of these drivers are older
than DT. This guessing was sufficient to make them work in the systems
of that time. Some drivers are used when DT is not available, e.g. USB
or PCI devices, or even ACPI.
> There is also a case for DSA. If there's an implication that the DSA
> controlled switch has an MDIO bus (phy_read() and phy_write()), the DSA
> driver will connect the switch MACs to the PHYs on the MDIO bus of the
> switch, even if there's no description of that MDIO bus on the devicetree.
> As unlikely as it is on a real life scenario, there may be a device that
> has its switch MACs wired to the PHYs on another MDIO bus.
> This is why I've proposed to make the drivers strictly follow what the
> devicetree says.
There are mv88e6xxx systems which don't have a DT description, just
platform data. So if you need to make code changes, keep that in mind.
Andrew
On 23.09.2023 18:12, Andrew Lunn wrote:
> On Sat, Sep 23, 2023 at 09:28:41AM +0300, Arınç ÜNAL wrote:
>> On 23.09.2023 01:36, Andrew Lunn wrote:
>>> You are missing:
>>>
>>> - The MAC has firmware driving the PHY, nothing for linux to do.
>>>
>>> There are properties in ethernet-controller.yaml the MAC driver would
>>> however like to use such as local-mac-address, max-frame-size,
>>> nvmem-cell-names etc.
>>
>> This is interesting. This is clearly a hardware difference of the ethernet
>> controller.
>>
>> I believe this fits case 1. There's still an MDIO bus the ethernet
>> controller uses, there's still a PHY on the MDIO bus which the ethernet
>> controller uses.
>
> Why must there be an MDIO bus? All the bus provides is a communication
> channel to the PHY. There are PHYs which are memory mapped, or use
> I2C. SFP are a good example of I2C, which Linux maps to MDIO just to
> make things simple, but the hardware is I2C. Why must there be a PHY?
> Maybe it is a Base-K link, i.e. a baseboard link to a switch, or a BMC
> or something.
There's no requirement for an MDIO bus or a PHY. If the MAC is connected to
a PHY, only the PHY node needs to be described. The PHY can be controlled
by any interface, I2C, MDIO, or something else. If there's no PHY, the
fixed-link property would be used to describe the link.
>
>> The only difference is the firmware of the ethernet
>> controller controls... What exactly does the firmware control that a Linux
>> driver would have controlled instead? Just configuring the link settings of
>> the MAC?
>
> A MAC driver implements struct ethtool_ops:::get_link_settings and
> set_link_settings. For a MAC driver using phylib or phylink they
> typically then call into phylib or phylink to do the actual work,
> maybe with a bit of pre-processing in the MAC driver.
>
> A MAC driver using firmware would typically make an RPC into the
> firmware to implement these calls.
>
> There is a MAC driver currently under review which does not have a PHY
> at all. The MAC is directly connected to a switch, all within one
> IC. The link is always running at 5Gbps, the link is always up. It is
> physically impossible to connect a PHY, so get_link_settings just
> returns hard coded values.
The fixed-link property would be used to describe the link of the MAC here.
Arınç
On Sat, Sep 23, 2023 at 09:28:41AM +0300, Arınç ÜNAL wrote:
> On 23.09.2023 01:36, Andrew Lunn wrote:
> > > I agree. My patch description here failed to explain the actual issue,
> > > which is missing hardware descriptions. Here's what I understand. An
> > > ethernet-controller is a MAC. For the MAC to work properly with its link
> > > partner, at least one of these must be described:
> > > - pointer to a PHY to retrieve link information from the PHY
> > > - pointer to a PCS to retrieve link information from the PCS
> > > - pointer to an SFP to retrieve link information from the SFP
> > > - static link information
> >
> > You are missing:
> >
> > - The MAC has firmware driving the PHY, nothing for linux to do.
> >
> > There are properties in ethernet-controller.yaml the MAC driver would
> > however like to use such as local-mac-address, max-frame-size,
> > nvmem-cell-names etc.
>
> This is interesting. This is clearly a hardware difference of the ethernet
> controller.
>
> I believe this fits case 1. There's still an MDIO bus the ethernet
> controller uses, there's still a PHY on the MDIO bus which the ethernet
> controller uses.
Why must there be an MDIO bus? All the bus provides is a communication
channel to the PHY. There are PHYs which are memory mapped, or use
I2C. SFP are a good example of I2C, which Linux maps to MDIO just to
make things simple, but the hardware is I2C. Why must there be a PHY?
Maybe it is a Base-K link, i.e. a baseboard link to a switch, or a BMC
or something.
> The only difference is the firmware of the ethernet
> controller controls... What exactly does the firmware control that a Linux
> driver would have controlled instead? Just configuring the link settings of
> the MAC?
A MAC driver implements struct ethtool_ops:::get_link_settings and
set_link_settings. For a MAC driver using phylib or phylink they
typically then call into phylib or phylink to do the actual work,
maybe with a bit of pre-processing in the MAC driver.
A MAC driver using firmware would typically make an RPC into the
firmware to implement these calls.
There is a MAC driver currently under review which does not have a PHY
at all. The MAC is directly connected to a switch, all within one
IC. The link is always running at 5Gbps, the link is always up. It is
physically impossible to connect a PHY, so get_link_settings just
returns hard coded values.
Andrew
> > There is a MAC driver currently under review which does not have a PHY
> > at all. The MAC is directly connected to a switch, all within one
> > IC. The link is always running at 5Gbps, the link is always up. It is
> > physically impossible to connect a PHY, so get_link_settings just
> > returns hard coded values.
>
> The fixed-link property would be used to describe the link of the MAC here.
Fixed-link make sense for a general purpose MAC which could be
connected to a PHY, or could also be used without a PHY. fixed-link
simplifies the code in that the MAC driver does not see a difference,
it all looks like a PHY.
However for a MAC which cannot be connected to a PHY, there is no need
to emulate a PHY. The MAC driver will be simpler. So i would not
recommend a fixed-link in this situation.
Andrew
On 23.09.2023 01:44, Andrew Lunn wrote:
>> However, to dress this up as "phylink requires xyz, so lets create
>> a phylink binding description" is just wrong.
>
> +1
>
> Also, phylink is a Linux implementation detail. Other OSes using the
> binding don't need to have phylink. Yet they can still use the DT
> blobs because they should describe the hardware, independent of how
> the OS drives that hardware.
I haven't stated it directly but I've been agreeing to this fact since the
start of the discussion on patch 7.
Arınç
On 24/09/2023 06:15, Andrew Lunn wrote:
>>> There is a MAC driver currently under review which does not have a PHY
>>> at all. The MAC is directly connected to a switch, all within one
>>> IC. The link is always running at 5Gbps, the link is always up. It is
>>> physically impossible to connect a PHY, so get_link_settings just
>>> returns hard coded values.
>>
>> The fixed-link property would be used to describe the link of the MAC here.
>
> Fixed-link make sense for a general purpose MAC which could be
> connected to a PHY, or could also be used without a PHY. fixed-link
> simplifies the code in that the MAC driver does not see a difference,
> it all looks like a PHY.
>
> However for a MAC which cannot be connected to a PHY, there is no need
> to emulate a PHY. The MAC driver will be simpler. So i would not
> recommend a fixed-link in this situation.
There's a link, it must be described. The MAC driver can configure the link
without reading the fixed-link property as there's no room for guessing.
The phy-handle, pcs-handle, and sfp properties point out there's a PHY. The
fixed-link property can be used standalone to describe MAC to MAC links.
For this specific ethernet controller, the phy-handle, pcs-handle, and sfp
properties can be disallowed on its schema to point out the ethernet
controller cannot be connected to a PHY.
Arınç
On Sun, Sep 24, 2023 at 10:49:49AM +0300, Arınç ÜNAL wrote:
> On 24/09/2023 06:15, Andrew Lunn wrote:
> > > > There is a MAC driver currently under review which does not have a PHY
> > > > at all. The MAC is directly connected to a switch, all within one
> > > > IC. The link is always running at 5Gbps, the link is always up. It is
> > > > physically impossible to connect a PHY, so get_link_settings just
> > > > returns hard coded values.
> > >
> > > The fixed-link property would be used to describe the link of the MAC here.
> >
> > Fixed-link make sense for a general purpose MAC which could be
> > connected to a PHY, or could also be used without a PHY. fixed-link
> > simplifies the code in that the MAC driver does not see a difference,
> > it all looks like a PHY.
> >
> > However for a MAC which cannot be connected to a PHY, there is no need
> > to emulate a PHY. The MAC driver will be simpler. So i would not
> > recommend a fixed-link in this situation.
>
> There's a link, it must be described.
Why must it be described?
Lets take this to the extreme to make a point. The chip has a ground
pin. Must i describe that?
Andrew
On 24.09.2023 17:55, Andrew Lunn wrote:
> On Sun, Sep 24, 2023 at 10:49:49AM +0300, Arınç ÜNAL wrote:
>> On 24/09/2023 06:15, Andrew Lunn wrote:
>>>>> There is a MAC driver currently under review which does not have a PHY
>>>>> at all. The MAC is directly connected to a switch, all within one
>>>>> IC. The link is always running at 5Gbps, the link is always up. It is
>>>>> physically impossible to connect a PHY, so get_link_settings just
>>>>> returns hard coded values.
>>>>
>>>> The fixed-link property would be used to describe the link of the MAC here.
>>>
>>> Fixed-link make sense for a general purpose MAC which could be
>>> connected to a PHY, or could also be used without a PHY. fixed-link
>>> simplifies the code in that the MAC driver does not see a difference,
>>> it all looks like a PHY.
>>>
>>> However for a MAC which cannot be connected to a PHY, there is no need
>>> to emulate a PHY. The MAC driver will be simpler. So i would not
>>> recommend a fixed-link in this situation.
>>
>> There's a link, it must be described.
>
> Why must it be described?
>
> Lets take this to the extreme to make a point. The chip has a ground
> pin. Must i describe that?
I think it depends on how important the information is, to be put on the
devicetree. I don't think a ground pin of an SoC is important enough to be
described on the devicetree. It could be described as a text on the
relevant devicetree document though. I've recently submitted a patch that
does a similar thing. I've described which pin groups represent which pins.
https://lore.kernel.org/lkml/[email protected]/
For an ethernet controller, its link is the core part of the hardware.
Therefore describing the link was deemed important. Hence certain
properties were made to describe the link on the devicetree.
All I proposed was to make sure these properties are always defined on the
devicetree since, for an ethernet controller to exist, it must have a link.
Arınç