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

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç