[PATCH 1/2] mt7530: register OF node for internal MDIO bus

[PATCH 1/2] mt7530: register OF node for internal MDIO bus

[David Bauer]

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>
---
 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 c680873819b0..7e8ea5b75c3e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2119,10 +2119,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;
@@ -2141,7 +2144,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[David Bauer]

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

Signed-off-by: David Bauer <mail@david-bauer.net>
---
 .../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 e532c6b795f4..50f8f83cc440 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.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 30.04.2023 14:28, David Bauer wrote:
> Document the ability to add nodes for the MDIO bus connecting the
> switch-internal PHYs.

This is quite interesting. Currently the PHY muxing feature for the 
MT7530 switch looks for some fake ethernet-phy definitions on the 
mdio-bus where the switch is also defined.

Looking at the binding here, there will be an mdio node under the switch 
node. This could be useful to define the ethernet-phys for PHY muxing 
here instead, so we don't waste the register addresses on the parent 
mdio-bus for fake things. It looks like this should work right out of 
the box. I will do some tests.

Are there any examples as to what to configure on the switch PHYs with 
this change?

> 
> Signed-off-by: David Bauer <mail@david-bauer.net>
> ---
>   .../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 e532c6b795f4..50f8f83cc440 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.

Please set this property as false for mediatek,mt7988-switch as it 
doesn't use MDIO.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Golle]

On Sun, Apr 30, 2023 at 03:34:43PM +0300, Arınç ÜNAL wrote:
> On 30.04.2023 14:28, David Bauer wrote:
> > Document the ability to add nodes for the MDIO bus connecting the
> > switch-internal PHYs.
> 
> This is quite interesting. Currently the PHY muxing feature for the MT7530
> switch looks for some fake ethernet-phy definitions on the mdio-bus where
> the switch is also defined.
> 
> Looking at the binding here, there will be an mdio node under the switch
> node. This could be useful to define the ethernet-phys for PHY muxing here
> instead, so we don't waste the register addresses on the parent mdio-bus for
> fake things. It looks like this should work right out of the box. I will do
> some tests.
> 
> Are there any examples as to what to configure on the switch PHYs with this
> change?
> 
> > 
> > Signed-off-by: David Bauer <mail@david-bauer.net>
> > ---
> >   .../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 e532c6b795f4..50f8f83cc440 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.
> 
> Please set this property as false for mediatek,mt7988-switch as it doesn't
> use MDIO.

Well, quite the opposite is true. This change is **needed** on MT7988 as
the built-in 1GE PHYs of the MT7988 are connected to the (internal) MDIO
bus of the switch. And they do need calibration data assigned as nvmem
via device tree.

tl;dr: Despite not being connected via MDIO itself also MT7988 exposes an
internal MDIO bus for the switch PHYs.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 30.04.2023 15:44, Daniel Golle wrote:
> On Sun, Apr 30, 2023 at 03:34:43PM +0300, Arınç ÜNAL wrote:
>> On 30.04.2023 14:28, David Bauer wrote:
>>> Document the ability to add nodes for the MDIO bus connecting the
>>> switch-internal PHYs.
>>
>> This is quite interesting. Currently the PHY muxing feature for the MT7530
>> switch looks for some fake ethernet-phy definitions on the mdio-bus where
>> the switch is also defined.
>>
>> Looking at the binding here, there will be an mdio node under the switch
>> node. This could be useful to define the ethernet-phys for PHY muxing here
>> instead, so we don't waste the register addresses on the parent mdio-bus for
>> fake things. It looks like this should work right out of the box. I will do
>> some tests.
>>
>> Are there any examples as to what to configure on the switch PHYs with this
>> change?
>>
>>>
>>> Signed-off-by: David Bauer <mail@david-bauer.net>
>>> ---
>>>    .../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 e532c6b795f4..50f8f83cc440 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.
>>
>> Please set this property as false for mediatek,mt7988-switch as it doesn't
>> use MDIO.
> 
> Well, quite the opposite is true. This change is **needed** on MT7988 as
> the built-in 1GE PHYs of the MT7988 are connected to the (internal) MDIO
> bus of the switch. And they do need calibration data assigned as nvmem
> via device tree.
> 
> tl;dr: Despite not being connected via MDIO itself also MT7988 exposes an
> internal MDIO bus for the switch PHYs.

Got it, thanks.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 30.04.2023 15:34, Arınç ÜNAL wrote:
> On 30.04.2023 14:28, David Bauer wrote:
>> Document the ability to add nodes for the MDIO bus connecting the
>> switch-internal PHYs.
> 
> This is quite interesting. Currently the PHY muxing feature for the 
> MT7530 switch looks for some fake ethernet-phy definitions on the 
> mdio-bus where the switch is also defined.
> 
> Looking at the binding here, there will be an mdio node under the switch 
> node. This could be useful to define the ethernet-phys for PHY muxing 
> here instead, so we don't waste the register addresses on the parent 
> mdio-bus for fake things. It looks like this should work right out of 
> the box. I will do some tests.

Once I start using the mdio node it forces me to define all the PHYs 
which were defined as ports.

[    4.159534] mt7530-mdio mdio-bus:1f lan0 (uninitialized): no phy at 1
[    4.166002] mt7530-mdio mdio-bus:1f lan0 (uninitialized): failed to 
connect to PHY: -ENODEV
[    4.174421] mt7530-mdio mdio-bus:1f lan0 (uninitialized): error -19 
setting up PHY for tree 0, switch 0, port 1
[    4.185236] mt7530-mdio mdio-bus:1f lan1 (uninitialized): no phy at 2
[    4.191753] mt7530-mdio mdio-bus:1f lan1 (uninitialized): failed to 
connect to PHY: -ENODEV
[    4.200150] mt7530-mdio mdio-bus:1f lan1 (uninitialized): error -19 
setting up PHY for tree 0, switch 0, port 2
[    4.210844] mt7530-mdio mdio-bus:1f lan2 (uninitialized): no phy at 3
[    4.217361] mt7530-mdio mdio-bus:1f lan2 (uninitialized): failed to 
connect to PHY: -ENODEV
[    4.225734] mt7530-mdio mdio-bus:1f lan2 (uninitialized): error -19 
setting up PHY for tree 0, switch 0, port 3
[    4.236394] mt7530-mdio mdio-bus:1f lan3 (uninitialized): no phy at 4
[    4.242901] mt7530-mdio mdio-bus:1f lan3 (uninitialized): failed to 
connect to PHY: -ENODEV
[    4.251297] mt7530-mdio mdio-bus:1f lan3 (uninitialized): error -19 
setting up PHY for tree 0, switch 0, port 4

We can either force defining the PHYs on the mdio node which would break 
the ABI, or forget about doing PHY muxing this way.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andrew Lunn]

On Sun, Apr 30, 2023 at 07:17:10PM +0300, Arınç ÜNAL wrote:
> On 30.04.2023 15:34, Arınç ÜNAL wrote:
> > On 30.04.2023 14:28, David Bauer wrote:
> > > Document the ability to add nodes for the MDIO bus connecting the
> > > switch-internal PHYs.
> > 
> > This is quite interesting. Currently the PHY muxing feature for the
> > MT7530 switch looks for some fake ethernet-phy definitions on the
> > mdio-bus where the switch is also defined.
> > 
> > Looking at the binding here, there will be an mdio node under the switch
> > node. This could be useful to define the ethernet-phys for PHY muxing
> > here instead, so we don't waste the register addresses on the parent
> > mdio-bus for fake things. It looks like this should work right out of
> > the box. I will do some tests.
> 
> Once I start using the mdio node it forces me to define all the PHYs which
> were defined as ports.

Try setting ds->slave_mii_bus to the MDIO bus you register via
of_mdiobus_register().

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 30.04.2023 20:18, Andrew Lunn wrote:
> On Sun, Apr 30, 2023 at 07:17:10PM +0300, Arınç ÜNAL wrote:
>> On 30.04.2023 15:34, Arınç ÜNAL wrote:
>>> On 30.04.2023 14:28, David Bauer wrote:
>>>> Document the ability to add nodes for the MDIO bus connecting the
>>>> switch-internal PHYs.
>>>
>>> This is quite interesting. Currently the PHY muxing feature for the
>>> MT7530 switch looks for some fake ethernet-phy definitions on the
>>> mdio-bus where the switch is also defined.
>>>
>>> Looking at the binding here, there will be an mdio node under the switch
>>> node. This could be useful to define the ethernet-phys for PHY muxing
>>> here instead, so we don't waste the register addresses on the parent
>>> mdio-bus for fake things. It looks like this should work right out of
>>> the box. I will do some tests.
>>
>> Once I start using the mdio node it forces me to define all the PHYs which
>> were defined as ports.
> 
> Try setting ds->slave_mii_bus to the MDIO bus you register via
> of_mdiobus_register().

That seems to be the case already, under mt7530_setup_mdio():

	bus = devm_mdiobus_alloc(dev);
	if (!bus)
		return -ENOMEM;

	ds->slave_mii_bus = bus;

The bus is registered with devm_of_mdiobus_register(), if that matters. 
(My current knowledge about OF or OF helpers for MDIO is next to nothing.)

The same behaviour is there.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andrew Lunn]

> > Try setting ds->slave_mii_bus to the MDIO bus you register via
> > of_mdiobus_register().
> 
> That seems to be the case already, under mt7530_setup_mdio():
> 
> 	bus = devm_mdiobus_alloc(dev);
> 	if (!bus)
> 		return -ENOMEM;
> 
> 	ds->slave_mii_bus = bus;
> 
> The bus is registered with devm_of_mdiobus_register(), if that matters. (My
> current knowledge about OF or OF helpers for MDIO is next to nothing.)
> 
> The same behaviour is there.

Maybe take a look at what is going on in dsa_slave_phy_setup() and
dsa_slave_phy_connect().

The way i understand it, is it first looks in DT to see if there is a
phy-handle, and if there is, it uses it. If not, it assumes there is a
1:1 mapping between port number and PHY address, and looks to see if a
PHY has been found on ds->slave_mii_bus at that address, and uses it.

So i don't think you need to list the PHY, the fallback should be
used.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 30.04.2023 21:48, Andrew Lunn wrote:
>>> Try setting ds->slave_mii_bus to the MDIO bus you register via
>>> of_mdiobus_register().
>>
>> That seems to be the case already, under mt7530_setup_mdio():
>>
>> 	bus = devm_mdiobus_alloc(dev);
>> 	if (!bus)
>> 		return -ENOMEM;
>>
>> 	ds->slave_mii_bus = bus;
>>
>> The bus is registered with devm_of_mdiobus_register(), if that matters. (My
>> current knowledge about OF or OF helpers for MDIO is next to nothing.)
>>
>> The same behaviour is there.
> 
> Maybe take a look at what is going on in dsa_slave_phy_setup() and
> dsa_slave_phy_connect().
> 
> The way i understand it, is it first looks in DT to see if there is a
> phy-handle, and if there is, it uses it. If not, it assumes there is a
> 1:1 mapping between port number and PHY address, and looks to see if a
> PHY has been found on ds->slave_mii_bus at that address, and uses it.
> 
> So i don't think you need to list the PHY, the fallback should be
> used.

Thanks for pointing me in the right direction Andrew.

I applied this diff:

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 389f33a12534..19d0c209e7e9 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -117,8 +117,12 @@ struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
  
  	mdiodev = bus->mdio_map[addr];
  
-	if (!mdiodev)
+	if (!mdiodev) {
+		dev_info(&bus->dev, "mdio device doesn't exist\n");
  		return NULL;
+	}
+
+	dev_info(&bus->dev, "mdio device exists\n");
  
  	if (!(mdiodev->flags & MDIO_DEVICE_FLAG_PHY))
  		return NULL;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 165bb2cb8431..0be408e32a76 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2487,6 +2487,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
  		/* We could not connect to a designated PHY or SFP, so try to
  		 * use the switch internal MDIO bus instead
  		 */
+		netdev_err(slave_dev, "using switch's internal MDIO bus\n");
  		ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
  	}
  	if (ret) {

With or without this patch, the switch's internal MDIO bus is used to set
up the PHYs.

DT that defines ethphy0 only, without this patch applied:

[    4.660784] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
[    4.669026] mdio_bus mt7530-0: mdio device exists
[    4.677693] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.693238] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
[    4.701589] mdio_bus mt7530-0: mdio device exists
[    4.707101] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.718550] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
[    4.726856] mdio_bus mt7530-0: mdio device exists
[    4.732384] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.743822] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
[    4.752154] mdio_bus mt7530-0: mdio device exists
[    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.769099] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
[    4.781872] mdio_bus mt7530-0: mdio device exists
[    4.787413] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)

Same DT but with this patch applied:

[    4.621547] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
[    4.631524] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
[    4.639764] mdio_bus mt7530-0: mdio device exists
[    4.647770] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.663898] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
[    4.672253] mdio_bus mt7530-0: mdio device doesn't exist
[    4.677597] mt7530-mdio mdio-bus:1f lan0 (uninitialized): no phy at 1
[    4.684053] mt7530-mdio mdio-bus:1f lan0 (uninitialized): failed to connect to PHY: -ENODEV
[    4.692435] mt7530-mdio mdio-bus:1f lan0 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 1
[    4.703087] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
[    4.711408] mdio_bus mt7530-0: mdio device doesn't exist
[    4.716731] mt7530-mdio mdio-bus:1f lan1 (uninitialized): no phy at 2
[    4.723214] mt7530-mdio mdio-bus:1f lan1 (uninitialized): failed to connect to PHY: -ENODEV
[    4.731597] mt7530-mdio mdio-bus:1f lan1 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 2
[    4.742199] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
[    4.755431] mdio_bus mt7530-0: mdio device doesn't exist
[    4.760793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): no phy at 3
[    4.767263] mt7530-mdio mdio-bus:1f lan2 (uninitialized): failed to connect to PHY: -ENODEV
[    4.775632] mt7530-mdio mdio-bus:1f lan2 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3
[    4.786270] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
[    4.794591] mdio_bus mt7530-0: mdio device doesn't exist
[    4.799944] mt7530-mdio mdio-bus:1f lan3 (uninitialized): no phy at 4
[    4.806397] mt7530-mdio mdio-bus:1f lan3 (uninitialized): failed to connect to PHY: -ENODEV
[    4.814782] mt7530-mdio mdio-bus:1f lan3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 4

DT without the mdio node defined, with this patch applied:

[    4.650766] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
[    4.660687] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
[    4.668937] mdio_bus mt7530-0: mdio device exists
[    4.677787] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.693165] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
[    4.701517] mdio_bus mt7530-0: mdio device exists
[    4.707029] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.718469] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
[    4.726773] mdio_bus mt7530-0: mdio device exists
[    4.732322] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.743793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
[    4.752143] mdio_bus mt7530-0: mdio device exists
[    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.769105] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
[    4.781905] mdio_bus mt7530-0: mdio device exists
[    4.787459] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)

This is how I define it, mind you no phandles.

switch@1f {
	...
	mdio {
		#address-cells = <0x01>;
		#size-cells = <0x00>;

		ethernet-phy@0 {
			reg = <0x00>;
		};
	};
};

Like you said, if the mdio node is not defined, the driver will assume 1:1
mapping. If not, it will need all the PHYs to be defined on the mdio node
along with on the ports node. Hence back to my original statement, we can
either force defining the PHYs on the mdio node which would break the ABI,
or forget about doing PHY muxing this way.

There are no MDIO operations needed on the PHYs for the PHY muxing anyway,
so I'd rather do this some other way.

Thanks for the help.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andrew Lunn]

On Sun, Apr 30, 2023 at 10:54:50PM +0300, Arınç ÜNAL wrote:
> On 30.04.2023 21:48, Andrew Lunn wrote:
> > > > Try setting ds->slave_mii_bus to the MDIO bus you register via
> > > > of_mdiobus_register().
> > > 
> > > That seems to be the case already, under mt7530_setup_mdio():
> > > 
> > > 	bus = devm_mdiobus_alloc(dev);
> > > 	if (!bus)
> > > 		return -ENOMEM;
> > > 
> > > 	ds->slave_mii_bus = bus;
> > > 
> > > The bus is registered with devm_of_mdiobus_register(), if that matters. (My
> > > current knowledge about OF or OF helpers for MDIO is next to nothing.)
> > > 
> > > The same behaviour is there.
> > 
> > Maybe take a look at what is going on in dsa_slave_phy_setup() and
> > dsa_slave_phy_connect().
> > 
> > The way i understand it, is it first looks in DT to see if there is a
> > phy-handle, and if there is, it uses it. If not, it assumes there is a
> > 1:1 mapping between port number and PHY address, and looks to see if a
> > PHY has been found on ds->slave_mii_bus at that address, and uses it.
> > 
> > So i don't think you need to list the PHY, the fallback should be
> > used.
> 
> Thanks for pointing me in the right direction Andrew.
> 
> I applied this diff:
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 389f33a12534..19d0c209e7e9 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -117,8 +117,12 @@ struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
>  	mdiodev = bus->mdio_map[addr];
> -	if (!mdiodev)
> +	if (!mdiodev) {
> +		dev_info(&bus->dev, "mdio device doesn't exist\n");
>  		return NULL;
> +	}
> +
> +	dev_info(&bus->dev, "mdio device exists\n");
>  	if (!(mdiodev->flags & MDIO_DEVICE_FLAG_PHY))
>  		return NULL;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 165bb2cb8431..0be408e32a76 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2487,6 +2487,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
>  		/* We could not connect to a designated PHY or SFP, so try to
>  		 * use the switch internal MDIO bus instead
>  		 */
> +		netdev_err(slave_dev, "using switch's internal MDIO bus\n");
>  		ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
>  	}
>  	if (ret) {
> 
> With or without this patch, the switch's internal MDIO bus is used to set
> up the PHYs.
> 
> DT that defines ethphy0 only, without this patch applied:
> 
> [    4.660784] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
> [    4.669026] mdio_bus mt7530-0: mdio device exists
> [    4.677693] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.693238] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
> [    4.701589] mdio_bus mt7530-0: mdio device exists
> [    4.707101] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.718550] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
> [    4.726856] mdio_bus mt7530-0: mdio device exists
> [    4.732384] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.743822] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
> [    4.752154] mdio_bus mt7530-0: mdio device exists
> [    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.769099] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
> [    4.781872] mdio_bus mt7530-0: mdio device exists
> [    4.787413] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> Same DT but with this patch applied:
> 
> [    4.621547] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
> [    4.631524] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
> [    4.639764] mdio_bus mt7530-0: mdio device exists
> [    4.647770] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.663898] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
> [    4.672253] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.677597] mt7530-mdio mdio-bus:1f lan0 (uninitialized): no phy at 1
> [    4.684053] mt7530-mdio mdio-bus:1f lan0 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.692435] mt7530-mdio mdio-bus:1f lan0 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 1
> [    4.703087] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
> [    4.711408] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.716731] mt7530-mdio mdio-bus:1f lan1 (uninitialized): no phy at 2
> [    4.723214] mt7530-mdio mdio-bus:1f lan1 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.731597] mt7530-mdio mdio-bus:1f lan1 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 2
> [    4.742199] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
> [    4.755431] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.760793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): no phy at 3
> [    4.767263] mt7530-mdio mdio-bus:1f lan2 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.775632] mt7530-mdio mdio-bus:1f lan2 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3
> [    4.786270] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
> [    4.794591] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.799944] mt7530-mdio mdio-bus:1f lan3 (uninitialized): no phy at 4
> [    4.806397] mt7530-mdio mdio-bus:1f lan3 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.814782] mt7530-mdio mdio-bus:1f lan3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 4
> 
> DT without the mdio node defined, with this patch applied:
> 
> [    4.650766] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
> [    4.660687] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
> [    4.668937] mdio_bus mt7530-0: mdio device exists
> [    4.677787] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.693165] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
> [    4.701517] mdio_bus mt7530-0: mdio device exists
> [    4.707029] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.718469] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
> [    4.726773] mdio_bus mt7530-0: mdio device exists
> [    4.732322] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.743793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
> [    4.752143] mdio_bus mt7530-0: mdio device exists
> [    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.769105] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
> [    4.781905] mdio_bus mt7530-0: mdio device exists
> [    4.787459] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> This is how I define it, mind you no phandles.
> 
> switch@1f {
> 	...
> 	mdio {
> 		#address-cells = <0x01>;
> 		#size-cells = <0x00>;
> 
> 		ethernet-phy@0 {
> 			reg = <0x00>;
> 		};
> 	};
> };
> 
> Like you said, if the mdio node is not defined, the driver will assume 1:1
> mapping. If not, it will need all the PHYs to be defined on the mdio node
> along with on the ports node. Hence back to my original statement, we can
> either force defining the PHYs on the mdio node which would break the ABI,
> or forget about doing PHY muxing this way.
> 
> There are no MDIO operations needed on the PHYs for the PHY muxing anyway,
> so I'd rather do this some other way.

The problem is __of_mdiobus_register() :-(

If there is a node in DT, the scanning of all addresses is
disabled. It looks at just the addresses listed in DT. Plain
mdiobus_register() by default looks at all addresses to discover if a
PHY is there.

Hence when slave.c tries to connect to the PHY, only the PHY you
listed in DT has been found.

I don't see a way around this, at least not without adding a new
variant of of_mdiobus_register which combined DT with a bus scan.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[David Bauer]

Hi Arinc,

thanks for spotting this issue.

On 4/30/23 21:54, Arınç ÜNAL wrote:
> On 30.04.2023 21:48, Andrew Lunn wrote:
>>>> Try setting ds->slave_mii_bus to the MDIO bus you register via
>>>> of_mdiobus_register().
>>>
>>> That seems to be the case already, under mt7530_setup_mdio():
>>>
>>>     bus = devm_mdiobus_alloc(dev);
>>>     if (!bus)
>>>         return -ENOMEM;
>>>
>>>     ds->slave_mii_bus = bus;
>>>
>>> The bus is registered with devm_of_mdiobus_register(), if that matters. (My
>>> current knowledge about OF or OF helpers for MDIO is next to nothing.)
>>>
>>> The same behaviour is there.
>>
>> Maybe take a look at what is going on in dsa_slave_phy_setup() and
>> dsa_slave_phy_connect().
>>
>> The way i understand it, is it first looks in DT to see if there is a
>> phy-handle, and if there is, it uses it. If not, it assumes there is a
>> 1:1 mapping between port number and PHY address, and looks to see if a
>> PHY has been found on ds->slave_mii_bus at that address, and uses it.
>>
>> So i don't think you need to list the PHY, the fallback should be
>> used.
> 
> Thanks for pointing me in the right direction Andrew.
> 
> I applied this diff:
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 389f33a12534..19d0c209e7e9 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -117,8 +117,12 @@ struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
> 
>       mdiodev = bus->mdio_map[addr];
> 
> -    if (!mdiodev)
> +    if (!mdiodev) {
> +        dev_info(&bus->dev, "mdio device doesn't exist\n");
>           return NULL;
> +    }
> +
> +    dev_info(&bus->dev, "mdio device exists\n");
> 
>       if (!(mdiodev->flags & MDIO_DEVICE_FLAG_PHY))
>           return NULL;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 165bb2cb8431..0be408e32a76 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2487,6 +2487,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
>           /* We could not connect to a designated PHY or SFP, so try to
>            * use the switch internal MDIO bus instead
>            */
> +        netdev_err(slave_dev, "using switch's internal MDIO bus\n");
>           ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
>       }
>       if (ret) {
> 
> With or without this patch, the switch's internal MDIO bus is used to set
> up the PHYs.
> 
> DT that defines ethphy0 only, without this patch applied:
> 
> [    4.660784] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
> [    4.669026] mdio_bus mt7530-0: mdio device exists
> [    4.677693] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.693238] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
> [    4.701589] mdio_bus mt7530-0: mdio device exists
> [    4.707101] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.718550] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
> [    4.726856] mdio_bus mt7530-0: mdio device exists
> [    4.732384] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.743822] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
> [    4.752154] mdio_bus mt7530-0: mdio device exists
> [    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.769099] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
> [    4.781872] mdio_bus mt7530-0: mdio device exists
> [    4.787413] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> Same DT but with this patch applied:
> 
> [    4.621547] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
> [    4.631524] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
> [    4.639764] mdio_bus mt7530-0: mdio device exists
> [    4.647770] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.663898] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
> [    4.672253] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.677597] mt7530-mdio mdio-bus:1f lan0 (uninitialized): no phy at 1
> [    4.684053] mt7530-mdio mdio-bus:1f lan0 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.692435] mt7530-mdio mdio-bus:1f lan0 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 1
> [    4.703087] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
> [    4.711408] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.716731] mt7530-mdio mdio-bus:1f lan1 (uninitialized): no phy at 2
> [    4.723214] mt7530-mdio mdio-bus:1f lan1 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.731597] mt7530-mdio mdio-bus:1f lan1 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 2
> [    4.742199] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
> [    4.755431] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.760793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): no phy at 3
> [    4.767263] mt7530-mdio mdio-bus:1f lan2 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.775632] mt7530-mdio mdio-bus:1f lan2 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3
> [    4.786270] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
> [    4.794591] mdio_bus mt7530-0: mdio device doesn't exist
> [    4.799944] mt7530-mdio mdio-bus:1f lan3 (uninitialized): no phy at 4
> [    4.806397] mt7530-mdio mdio-bus:1f lan3 (uninitialized): failed to connect to PHY: -ENODEV
> [    4.814782] mt7530-mdio mdio-bus:1f lan3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 4
> 
> DT without the mdio node defined, with this patch applied:
> 
> [    4.650766] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
> [    4.660687] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
> [    4.668937] mdio_bus mt7530-0: mdio device exists
> [    4.677787] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.693165] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
> [    4.701517] mdio_bus mt7530-0: mdio device exists
> [    4.707029] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.718469] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
> [    4.726773] mdio_bus mt7530-0: mdio device exists
> [    4.732322] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.743793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
> [    4.752143] mdio_bus mt7530-0: mdio device exists
> [    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.769105] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
> [    4.781905] mdio_bus mt7530-0: mdio device exists
> [    4.787459] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> This is how I define it, mind you no phandles.
> 
> switch@1f {
>      ...
>      mdio {
>          #address-cells = <0x01>;
>          #size-cells = <0x00>;
> 
>          ethernet-phy@0 {
>              reg = <0x00>;
>          };
>      };
> };
> 
> Like you said, if the mdio node is not defined, the driver will assume 1:1
> mapping. If not, it will need all the PHYs to be defined on the mdio node
> along with on the ports node. Hence back to my original statement, we can
> either force defining the PHYs on the mdio node which would break the ABI,
> or forget about doing PHY muxing this way.

While i was not aware of this side effect, I don't see how this breaks the ABI.

Existing device-trees not defining the MDIO node will still continue to work.

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?

Best
David

> 
> There are no MDIO operations needed on the PHYs for the PHY muxing anyway,
> so I'd rather do this some other way.
> 
> Thanks for the help.
> 
> Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 1.05.2023 12:22, David Bauer wrote:
> Hi Arinc,
> 
> thanks for spotting this issue.
> 
> On 4/30/23 21:54, Arınç ÜNAL wrote:
>> On 30.04.2023 21:48, Andrew Lunn wrote:
>>>>> Try setting ds->slave_mii_bus to the MDIO bus you register via
>>>>> of_mdiobus_register().
>>>>
>>>> That seems to be the case already, under mt7530_setup_mdio():
>>>>
>>>>     bus = devm_mdiobus_alloc(dev);
>>>>     if (!bus)
>>>>         return -ENOMEM;
>>>>
>>>>     ds->slave_mii_bus = bus;
>>>>
>>>> The bus is registered with devm_of_mdiobus_register(), if that matters. (My
>>>> current knowledge about OF or OF helpers for MDIO is next to nothing.)
>>>>
>>>> The same behaviour is there.
>>>
>>> Maybe take a look at what is going on in dsa_slave_phy_setup() and
>>> dsa_slave_phy_connect().
>>>
>>> The way i understand it, is it first looks in DT to see if there is a
>>> phy-handle, and if there is, it uses it. If not, it assumes there is a
>>> 1:1 mapping between port number and PHY address, and looks to see if a
>>> PHY has been found on ds->slave_mii_bus at that address, and uses it.
>>>
>>> So i don't think you need to list the PHY, the fallback should be
>>> used.
>>
>> Thanks for pointing me in the right direction Andrew.
>>
>> I applied this diff:
>>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 389f33a12534..19d0c209e7e9 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -117,8 +117,12 @@ struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
>>
>>       mdiodev = bus->mdio_map[addr];
>>
>> -    if (!mdiodev)
>> +    if (!mdiodev) {
>> +        dev_info(&bus->dev, "mdio device doesn't exist\n");
>>           return NULL;
>> +    }
>> +
>> +    dev_info(&bus->dev, "mdio device exists\n");
>>
>>       if (!(mdiodev->flags & MDIO_DEVICE_FLAG_PHY))
>>           return NULL;
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 165bb2cb8431..0be408e32a76 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -2487,6 +2487,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
>>           /* We could not connect to a designated PHY or SFP, so try to
>>            * use the switch internal MDIO bus instead
>>            */
>> +        netdev_err(slave_dev, "using switch's internal MDIO bus\n");
>>           ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
>>       }
>>       if (ret) {
>>
>> With or without this patch, the switch's internal MDIO bus is used to set
>> up the PHYs.
>>
>> DT that defines ethphy0 only, without this patch applied:
>>
>> [    4.660784] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
>> [    4.669026] mdio_bus mt7530-0: mdio device exists
>> [    4.677693] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.693238] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
>> [    4.701589] mdio_bus mt7530-0: mdio device exists
>> [    4.707101] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.718550] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
>> [    4.726856] mdio_bus mt7530-0: mdio device exists
>> [    4.732384] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.743822] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
>> [    4.752154] mdio_bus mt7530-0: mdio device exists
>> [    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.769099] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
>> [    4.781872] mdio_bus mt7530-0: mdio device exists
>> [    4.787413] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
>>
>> Same DT but with this patch applied:
>>
>> [    4.621547] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
>> [    4.631524] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
>> [    4.639764] mdio_bus mt7530-0: mdio device exists
>> [    4.647770] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.663898] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
>> [    4.672253] mdio_bus mt7530-0: mdio device doesn't exist
>> [    4.677597] mt7530-mdio mdio-bus:1f lan0 (uninitialized): no phy at 1
>> [    4.684053] mt7530-mdio mdio-bus:1f lan0 (uninitialized): failed to connect to PHY: -ENODEV
>> [    4.692435] mt7530-mdio mdio-bus:1f lan0 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 1
>> [    4.703087] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
>> [    4.711408] mdio_bus mt7530-0: mdio device doesn't exist
>> [    4.716731] mt7530-mdio mdio-bus:1f lan1 (uninitialized): no phy at 2
>> [    4.723214] mt7530-mdio mdio-bus:1f lan1 (uninitialized): failed to connect to PHY: -ENODEV
>> [    4.731597] mt7530-mdio mdio-bus:1f lan1 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 2
>> [    4.742199] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
>> [    4.755431] mdio_bus mt7530-0: mdio device doesn't exist
>> [    4.760793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): no phy at 3
>> [    4.767263] mt7530-mdio mdio-bus:1f lan2 (uninitialized): failed to connect to PHY: -ENODEV
>> [    4.775632] mt7530-mdio mdio-bus:1f lan2 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3
>> [    4.786270] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
>> [    4.794591] mdio_bus mt7530-0: mdio device doesn't exist
>> [    4.799944] mt7530-mdio mdio-bus:1f lan3 (uninitialized): no phy at 4
>> [    4.806397] mt7530-mdio mdio-bus:1f lan3 (uninitialized): failed to connect to PHY: -ENODEV
>> [    4.814782] mt7530-mdio mdio-bus:1f lan3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 4
>>
>> DT without the mdio node defined, with this patch applied:
>>
>> [    4.650766] mt7530-mdio mdio-bus:1f: configuring for fixed/trgmii link mode
>> [    4.660687] mt7530-mdio mdio-bus:1f wan (uninitialized): using switch's internal MDIO bus
>> [    4.668937] mdio_bus mt7530-0: mdio device exists
>> [    4.677787] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.693165] mt7530-mdio mdio-bus:1f lan0 (uninitialized): using switch's internal MDIO bus
>> [    4.701517] mdio_bus mt7530-0: mdio device exists
>> [    4.707029] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.718469] mt7530-mdio mdio-bus:1f lan1 (uninitialized): using switch's internal MDIO bus
>> [    4.726773] mdio_bus mt7530-0: mdio device exists
>> [    4.732322] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.743793] mt7530-mdio mdio-bus:1f lan2 (uninitialized): using switch's internal MDIO bus
>> [    4.752143] mdio_bus mt7530-0: mdio device exists
>> [    4.757662] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.769105] mt7530-mdio mdio-bus:1f lan3 (uninitialized): using switch's internal MDIO bus
>> [    4.781905] mdio_bus mt7530-0: mdio device exists
>> [    4.787459] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
>>
>> This is how I define it, mind you no phandles.
>>
>> switch@1f {
>>      ...
>>      mdio {
>>          #address-cells = <0x01>;
>>          #size-cells = <0x00>;
>>
>>          ethernet-phy@0 {
>>              reg = <0x00>;
>>          };
>>      };
>> };
>>
>> Like you said, if the mdio node is not defined, the driver will assume 1:1
>> mapping. If not, it will need all the PHYs to be defined on the mdio node
>> along with on the ports node. Hence back to my original statement, we can
>> either force defining the PHYs on the mdio node which would break the ABI,
>> or forget about doing PHY muxing this way.
> 
> 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.

> 
> Existing device-trees not defining the MDIO node will still continue to work.

Agreed, I also confirmed this with my test above.

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

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 30.04.2023 14:28, David Bauer wrote:
> Document the ability to add nodes for the MDIO bus connecting the
> switch-internal PHYs.
> 
> Signed-off-by: David Bauer <mail@david-bauer.net>
> ---
>   .../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 e532c6b795f4..50f8f83cc440 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.

Maybe saying "connected to the internal switch PHYs" would better 
explain this.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] mt7530: register OF node for internal MDIO bus

[Daniel Golle]

On Sun, Apr 30, 2023 at 01:28:32PM +0200, David Bauer wrote:
> 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>

Acked-by: Daniel Golle <daniel@makrotopia.org>

I've got a similar commit in my tree for a while, yours is better
because you care to of_node_put(mnp) ;)
And yes, this will be fine also if mnp is NULL, the MDIO bus will
still be registered (just without the reference to the OF node) and
also of_node_put is fine being called for NULL.

> ---
>  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 c680873819b0..7e8ea5b75c3e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2119,10 +2119,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;
> @@ -2141,7 +2144,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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] mt7530: register OF node for internal MDIO bus

[Andrew Lunn]

On Sun, Apr 30, 2023 at 01:28:32PM +0200, David Bauer wrote:
> 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>

Hi David

Please read

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

You should put the tree in the patch subject.

Also, net-next is closed at the moment due to the merge window. Please
only post RFC patches during this time, and repost once net-next opens
again.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel