Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus

[Vladimir Oltean]

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@xxxxxxxxxx/
> > > 
> > > 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.

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?
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.