[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Grygorii Strashko]

On 21/11/2019 12:36, Alexandru Marginean wrote:
> On 11/20/2019 10:51 AM, Grygorii Strashko wrote:
>>
>>
>> On 19/11/2019 20:58, Grygorii Strashko wrote:
>>>
>>>
>>> On 19/11/2019 01:31, Alexandru Marginean wrote:
>>>>
>>>> On 11/18/2019 8:11 PM, Grygorii Strashko wrote:
>>>>>
>>>>>
>>>>> On 14/11/2019 17:04, Alex Marginean wrote:
>>>>>> Adds an optional mdio-handle property which identifies a MDIO bus which can
>>>>>> be scanned to find the relevant PHY.  The property is ignored if phy-handle
>>>>>> is also present.
>>>>>>
>>>>>> Signed-off-by: Alex Marginean <alexandru.marginean at nxp.com>
>>>>>> ---
>>>>>>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>>>>>>   1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt
>>>>>> index 3fc360523b..9f9629f8d6 100644
>>>>>> --- a/doc/device-tree-bindings/net/ethernet.txt
>>>>>> +++ b/doc/device-tree-bindings/net/ethernet.txt
>>>>>> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet controllers:
>>>>>>   - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
>>>>>>   - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
>>>>>>     the maximum frame size (there's contradiction in ePAPR).
>>>>>> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be scanned to
>>>>>> +  find the PHY device.  Ignored if phy-handle is also present.
>>>>>
>>>>> Sry, but it looks redundant. The Ethernet-controller bindings
>>>>> expects to use phy-handle which, in turn, allows to get MDIO node.
>>>>
>>>> The problem I'm trying to solve is not that I don't want to get the
>>>> parent MDIO bus of a PHY referenced though phy-handle, but rather that I
>>>> want to avoid having a specific U-Boot DT for each PHY plug-in card that
>>>> could be used in our systems.  I'll explain that in more detail.
>>>>
>>>> We have these QDS systems which support plug-in cards with PHYs on them.
>>>> The way it works is that a given ethernet interface is associated with
>>>> one of the QDS slots that should contain a plug-in card.  Each of the
>>>> slots has a MDIO bus associated with it, this bus is accessed through a
>>>> MDIO MUX on the QDS board.
>>>> The slot may be populated with one of several types of cards, which
>>>> are different designs, have different types of PHYs and more importantly
>>>> use different PHY addresses on the MDIO bus.
>>>>
>>>> So now the summary is that an Ethernet interface is associated with a
>>>> MDIO bus that could be scanned to find the relevant PHY, while using the
>>>> existing phy-handle binding requires that U-Boot DT is edited whenever
>>>> a different card is plugged in, which is not practical and avoidable.
>>>
>>> Thank you for explanation. It's clear now.
>>> I think above description is good to have as part of commit message.
>>>
>>>>
>>>>>
>>>>> So, if your platform is DT based and can use DT then it's reasonable to follow standard binding,
>>>>> which, in addition, allows to specify Ethernet PHY properties.
>>>>
>>>> Sure, and we do that on boards and for PHYs which are at fixed addresses
>>>> on MDIO bus, but that is impractical with the swappable cards.
>>>>
>>
>> Some thought, which i think might help.
>> - u-boot allows phy node not to have "reg" property.
>> - phy_connect() will return first PHY discovered on the MDIO bus if addr<=0
>>
>> So, if MDIO assignment per ethernet interface/slot is fixed DT can look like
>>
>> mdioX {
>>      mdiox_phy0: ethernet-phy at 0 {
>>      };
>> }
>>
>> ethernetX {
>>      phy-handle = <&mdiox_phy0>;
>> }
>>
>> and so on. driver's parsing code can ignore PHY "reg" prop in phy
>> node and pass addr<=0 to phy_connect().
> Functionally that's what I'm looking for, yes.  There is the problem of
> not strictly following the kernel binding and at the end of the day I
> will have the same problem with the kernel too, so I should take this
> topic to the netdev list anyway.
> 
> Having 'reg' made optional is actually an interesting idea.  I think
> 'mdio-handle' is a bit more clear what it is, but having an actual PHY
> node allows passing DT properties on to the PHY driver which is
> certainly useful for some PHYs.  It's like saying I don't know what PHY
> I'm going to find, but if it's a PHY that has these configurable
> properties here is your configuration.

Another good question is "phy-connection-type"/"phy-mode".
Are all your cards works in the same mode? And how is this solved?

Is there anything common with SFP? Linux: bindings/net/sff,sfp.txt

> 
>>
>> It seems, current Linux approach is to have PHY "reg" property as required, but your case might be the reason to change this. >
> I'll have to do a better write-up for the kernel list and we'll see
> how this will go.
> In the meantime any feedback on the other patches, except the one
> introducing mdio-handle?  For instance I should deal with fixed links
> in the dm_eth_phy_connect helper too (calling phy_connect_fixed),
> my intent is to take some code that is otherwise pretty generic out of
> the drivers.

 From one side, it look nice. From another, in my case MDIO is not a device,
so can't try it.

But, any way, I'd like to share some notes I have. It may help you or may
save your time by reducing number of possible regressions.

To be honest, there is a little mess in PHY creation area :(
Common way is to do in drivers:
phy_connect(mii_bus, phy_addr, dev, interface)
  |-phy_find_by_mask()
      |-get_phy_device_by_mask()
	|-create_phy_by_mask()
	  |-create_phy_by_mask()
             |-phy_device_create()
		|-phy_probe()
  |-phy_connect_dev(phydev, dev);
..
#ifdef CONFIG_DM_ETH
	if (ofnode_valid(slave->data->phy_of_handle))
		phydev->node = slave->data->phy_of_handle;
#endif

As you can see from above - the PHY probe will be called when
both phy_device->dev and phydev->node have not been initialized yet,
so no way to perform DT parsing in PHY .probe().

Another, not obvious, thing is that DT node for fixed phy passed
through the int addr parameter in create_phy_by_mask().

And finally, as phy as fixed-phy may be connected to DT node which is
*not" ethernet device node and describes Port. The ethernet device is parent DT node
in this case.

In my opinion, it could really simplify things if phy_device->dev and phydev->node will
be configured before calling PHY .probe(), but the challenge is to make it work
for !DT case.

-- 
Best regards,
grygorii