[PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
Arınç ÜNAL
arinc.unal at arinc9.com
Fri Aug 11 15:45:29 PDT 2023
On 10.08.2023 01:01, Vladimir Oltean wrote:
> On Wed, Aug 09, 2023 at 12:03:19PM +0300, Arınç ÜNAL wrote:
>> On 8.08.2023 15:17, Vladimir Oltean wrote:
>>> On Sat, Aug 05, 2023 at 11:15:15PM +0300, Arınç ÜNAL wrote:
>>>> I don't see a reason to resubmit this without addressing the requested
>>>> change.
>>>>
>>>>>> Wouldn't we just skip the whole issue by documenting the need for defining all PHYs
>>>>>> used on the switch when defining the MDIO bus?
>>>>>
>>>>> Good idea, please do that.
>>>>
>>>> https://lore.kernel.org/netdev/0f501bb6-18a0-1713-b08c-6ad244c022ec@arinc9.com/
>>>>
>>>> Arınç
>>>
>>> Arınç, where do you see that comment being added? AFAIU, it is a
>>> characteristic of the generic __of_mdiobus_register() code to set
>>> mdio->phy_mask = ~0, and nothing specific to the mt7530.
>>
>> What I believe is specific to DSA is, 1:1 mapping of the port reg to the
>> PHY reg on the mdio bus is disabled if the mdio bus is defined. Therefore,
>> I believe a notice like below fits mediatek,mt7530.yaml.
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> index e532c6b795f4..c59d58252cd5 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
>> @@ -128,6 +128,15 @@ properties:
>> See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt for
>> details for the regulator setup on these boards.
>> + mdio:
>> + $ref: /schemas/net/mdio.yaml#
>> + unevaluatedProperties: false
>> + description:
>> + Node for the internal MDIO bus connected to the embedded ethernet-PHYs.
>> + For every port defined under the "^(ethernet-)?ports$" node, a PHY must be
>> + defined under here and a phy-handle property must be defined under the
>> + port node to point to the PHY node.
>> +
>> mediatek,mcm:
>> type: boolean
>> description:
>>
>> Arınç
>
> In that case, putting the comment here would make more sense, no?
> (and maybe enforcing an actual schema, but I've no idea how to do that)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> index 480120469953..5a415f12f162 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> @@ -59,7 +59,14 @@ properties:
> - rtl8_4t
> - seville
>
> -# CPU and DSA ports must have phylink-compatible link descriptions
> +# CPU and DSA ports must have phylink-compatible link descriptions.
> +# On user ports, these are also supported, but are optional and may be omitted,
> +# meaning that these ports are implicitly connected to a PHY on an internal
> +# MDIO bus of the switch that isn't described in the device tree. If the switch
> +# does have a child node for the internal MDIO bus, the phylink-compatible
> +# bindings are also required (even if this is not enforced here). The detection
> +# of an internal MDIO bus is model-specific and may involve matching on the
> +# "mdio" node name or compatible string.
> if:
> oneOf:
> - required: [ ethernet ]
>
> Since commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), DSA as a
> framework also supports auto-creating an internal MDIO bus based on the
> presence of the "mdio" node name, so I guess it makes sense for the
> "mdio" to appear in the generic dsa.yaml if there's nothing else that's
> special about it.
I agree with this. I've done this which works. It's even found a port
node with the ethernet property missing, as it should've.
diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index ec74a660beda..03ccedbc49dc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -31,6 +31,24 @@ properties:
(single device hanging off a CPU port) must not specify this property
$ref: /schemas/types.yaml#/definitions/uint32-array
+ mdio:
+ description: The internal MDIO bus of the switch
+ $ref: /schemas/net/mdio.yaml#
+
+if:
+ required: [ mdio ]
+then:
+ patternProperties:
+ "^(ethernet-)?ports$":
+ patternProperties:
+ "^(ethernet-)?port@[0-9]+$":
+ if:
+ not:
+ required: [ ethernet ]
+ then:
+ required:
+ - phy-handle
+
additionalProperties: true
$defs:
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
index 8d7e878b84dc..fe1e2008995d 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
@@ -78,6 +78,16 @@ examples:
};
};
+ macb1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+ };
+
spi {
#address-cells = <1>;
#size-cells = <0>;
@@ -138,6 +148,7 @@ examples:
phy-mode = "rgmii";
tx-internal-delay-ps = <2000>;
rx-internal-delay-ps = <2000>;
+ ethernet = <&macb0>;
fixed-link {
speed = <1000>;
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cfd69c2604ea..f600e65fc990 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
title: Realtek switches for unmanaged switches
-allOf:
- - $ref: dsa.yaml#/$defs/ethernet-ports
-
maintainers:
- Linus Walleij <linus.walleij at linaro.org>
@@ -95,37 +92,41 @@ properties:
- '#address-cells'
- '#interrupt-cells'
- mdio:
- $ref: /schemas/net/mdio.yaml#
- unevaluatedProperties: false
-
- properties:
- compatible:
- const: realtek,smi-mdio
-
-if:
- required:
- - reg
-
-then:
- $ref: /schemas/spi/spi-peripheral-props.yaml#
- not:
- required:
- - mdc-gpios
- - mdio-gpios
- - mdio
-
- properties:
- mdc-gpios: false
- mdio-gpios: false
- mdio: false
-
-else:
- required:
- - mdc-gpios
- - mdio-gpios
- - mdio
- - reset-gpios
+allOf:
+ - $ref: dsa.yaml#/$defs/ethernet-ports
+ - if:
+ required: [ mdio ]
+ then:
+ properties:
+ mdio:
+ properties:
+ compatible:
+ const: realtek,smi-mdio
+
+ required:
+ - compatible
+
+ - if:
+ required:
+ - reg
+ then:
+ $ref: /schemas/spi/spi-peripheral-props.yaml#
+ not:
+ required:
+ - mdc-gpios
+ - mdio-gpios
+ - mdio
+
+ properties:
+ mdc-gpios: false
+ mdio-gpios: false
+ mdio: false
+ else:
+ required:
+ - mdc-gpios
+ - mdio-gpios
+ - mdio
+ - reset-gpios
required:
- compatible
>
> Also, in the earlier patch version you had replied to David Bauer:
>
> | > While i was not aware of this side effect, I don't see how this breaks the ABI.
> |
> | Your patch doesn't break it, my then-intention of doing PHY muxing by
> | utilising this would. Your first patch is perfectly fine as is.
>
> Could you please clarify what is your valid use case for not having a
> phy-handle to a PHY on an MDIO bus that is otherwise present in OF?
I had one possible use case, PHY muxing, but it has nothing to do with
the PHY registers of the PHYs on the internal MDIO bus so it's not a
valid use case.
> It doesn't _have_ to be broken. Since DSA knows the addresses of the
> internal PHYs, it can circumvent the lack of auto-scanning by manually
> calling get_phy_device() at the right (port-based) MDIO addresses.
> But any patch would need to have a clear reason before being considered
> for merging.
I think circumventing the mdio node for ports without a phy-handle will
bring unnecessary complexity and confusion as I may define a port with
reg = <1> with phy-handle to a phy with reg = <4>. In that case, a port
with reg = <4> without a phy-handle would try the phy with reg <4> and fail.
This is of course if I understood correctly that "(port-based) MDIO
addresses" means reading the MDIO address of a phy from the reg of a
port defined under the ports node.
Arınç
More information about the Linux-mediatek
mailing list