linux-mediatek.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND net-next 1/2] net: dsa: mt7530: register OF node for internal MDIO bus
@ 2023-08-05 14:42 Daniel Golle
  2023-08-05 14:43 ` [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus Daniel Golle
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Golle @ 2023-08-05 14:42 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Arınç ÜNAL,
	Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang, netdev,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek

From: David Bauer <mail@david-bauer.net>

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 <mail@david-bauer.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 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 8fbda739c1b35..9bb805de397a9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2152,10 +2152,13 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 {
 	struct dsa_switch *ds = priv->ds;
 	struct device *dev = priv->dev;
+	struct device_node *np, *mnp;
 	struct mii_bus *bus;
 	static int idx;
 	int ret;
 
+	np = priv->dev->of_node;
+
 	bus = devm_mdiobus_alloc(dev);
 	if (!bus)
 		return -ENOMEM;
@@ -2174,7 +2177,9 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	if (priv->irq)
 		mt7530_setup_mdio_irq(priv);
 
-	ret = devm_mdiobus_register(dev, bus);
+	mnp = of_get_child_by_name(np, "mdio");
+	ret = devm_of_mdiobus_register(dev, bus, mnp);
+	of_node_put(mnp);
 	if (ret) {
 		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
 		if (priv->irq)
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-05 14:42 [PATCH RESEND net-next 1/2] net: dsa: mt7530: register OF node for internal MDIO bus Daniel Golle
@ 2023-08-05 14:43 ` Daniel Golle
  2023-08-05 20:15   ` Arınç ÜNAL
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Golle @ 2023-08-05 14:43 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Arınç ÜNAL,
	Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang, netdev,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek

From: David Bauer <mail@david-bauer.net>

Document the ability to add nodes for the MDIO bus connecting the
switch-internal PHYs.

Signed-off-by: David Bauer <mail@david-bauer.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../devicetree/bindings/net/dsa/mediatek,mt7530.yaml        | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index e532c6b795f4f..50f8f83cc440f 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -128,6 +128,12 @@ 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.
+
   mediatek,mcm:
     type: boolean
     description:
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-05 14:43 ` [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus Daniel Golle
@ 2023-08-05 20:15   ` Arınç ÜNAL
  2023-08-08 12:17     ` Vladimir Oltean
  0 siblings, 1 reply; 9+ messages in thread
From: Arınç ÜNAL @ 2023-08-05 20:15 UTC (permalink / raw)
  To: Daniel Golle, Andrew Lunn, Florian Fainelli, Vladimir Oltean,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

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ç

On 5.08.2023 17:43, Daniel Golle wrote:
> From: David Bauer <mail@david-bauer.net>
> 
> Document the ability to add nodes for the MDIO bus connecting the
> switch-internal PHYs.
> 
> Signed-off-by: David Bauer <mail@david-bauer.net>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   .../devicetree/bindings/net/dsa/mediatek,mt7530.yaml        | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index e532c6b795f4f..50f8f83cc440f 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -128,6 +128,12 @@ 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.
> +
>     mediatek,mcm:
>       type: boolean
>       description:


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-05 20:15   ` Arınç ÜNAL
@ 2023-08-08 12:17     ` Vladimir Oltean
  2023-08-09  9:03       ` Arınç ÜNAL
  0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Oltean @ 2023-08-08 12:17 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Daniel Golle, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-08 12:17     ` Vladimir Oltean
@ 2023-08-09  9:03       ` Arınç ÜNAL
  2023-08-09 22:01         ` Vladimir Oltean
  0 siblings, 1 reply; 9+ messages in thread
From: Arınç ÜNAL @ 2023-08-09  9:03 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Daniel Golle, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

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ç


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-09  9:03       ` Arınç ÜNAL
@ 2023-08-09 22:01         ` Vladimir Oltean
  2023-08-11 22:45           ` Arınç ÜNAL
  0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Oltean @ 2023-08-09 22:01 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Daniel Golle, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

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.

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.


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-09 22:01         ` Vladimir Oltean
@ 2023-08-11 22:45           ` Arınç ÜNAL
  2023-11-26 22:35             ` Daniel Golle
  0 siblings, 1 reply; 9+ messages in thread
From: Arınç ÜNAL @ 2023-08-11 22:45 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Daniel Golle, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

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@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ç


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-08-11 22:45           ` Arınç ÜNAL
@ 2023-11-26 22:35             ` Daniel Golle
  2023-11-27 11:30               ` Arınç ÜNAL
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Golle @ 2023-11-26 22:35 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Vladimir Oltean, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

On Sat, Aug 12, 2023 at 01:45:29AM +0300, Arınç ÜNAL wrote:
> On 10.08.2023 01:01, Vladimir Oltean wrote:
> > [...]
> > 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.

Are you planning to complete/submit your work below?
I'm asking because being able to reference the PHYs on the internal
MDIO bus is mandatory on MT7988 which requires calibration data from
NVMEM for each PHY, so supporting MT7988 depends on the associated
driver change[1].

[1]: https://patchwork.kernel.org/project/netdevbpf/patch/6eb1b7b8dbc3a4b14becad15f0707d4f624ee18b.1691246461.git.daniel@makrotopia.org/


> 
> 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@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
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus
  2023-11-26 22:35             ` Daniel Golle
@ 2023-11-27 11:30               ` Arınç ÜNAL
  0 siblings, 0 replies; 9+ messages in thread
From: Arınç ÜNAL @ 2023-11-27 11:30 UTC (permalink / raw)
  To: Daniel Golle
  Cc: Vladimir Oltean, Andrew Lunn, Florian Fainelli, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
	AngeloGioacchino Del Regno, Landen Chao, DENG Qingfang,
	Sean Wang, netdev, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

On 27 November 2023 00:35:45 EET, Daniel Golle <daniel@makrotopia.org> wrote:
>On Sat, Aug 12, 2023 at 01:45:29AM +0300, Arınç ÜNAL wrote:
>> On 10.08.2023 01:01, Vladimir Oltean wrote:
>> > [...]
>> > 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.
>
>Are you planning to complete/submit your work below?
>I'm asking because being able to reference the PHYs on the internal
>MDIO bus is mandatory on MT7988 which requires calibration data from
>NVMEM for each PHY, so supporting MT7988 depends on the associated
>driver change[1].
>
>[1]: https://patchwork.kernel.org/project/netdevbpf/patch/6eb1b7b8dbc3a4b14becad15f0707d4f624ee18b.1691246461.git.daniel@makrotopia.org/

This patch triggered conversation on a deeper level regarding the
devicetree bindings aspect of it. I will continue discussing that with the
maintainers here and achieve a mutual agreement.

To get things going on the driver side, I think it's fine to submit that
as a single patch. I'll do that in a week, if nobody else does it first.

Arınç


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-11-27 11:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-05 14:42 [PATCH RESEND net-next 1/2] net: dsa: mt7530: register OF node for internal MDIO bus Daniel Golle
2023-08-05 14:43 ` [PATCH RESEND net-next 2/2] dt-bindings: net: dsa: mediatek,mt7530: document MDIO-bus Daniel Golle
2023-08-05 20:15   ` Arınç ÜNAL
2023-08-08 12:17     ` Vladimir Oltean
2023-08-09  9:03       ` Arınç ÜNAL
2023-08-09 22:01         ` Vladimir Oltean
2023-08-11 22:45           ` Arınç ÜNAL
2023-11-26 22:35             ` Daniel Golle
2023-11-27 11:30               ` Arınç ÜNAL

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).