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ç
