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

[Alexandru Marginean]

On 11/21/2019 1:30 PM, Grygorii Strashko wrote:
> 
>>>
>>> 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?

These SoCs generally support multiple protocols on the serdes lanes.
The protocol is selected at power on so just using a single static
DT isn't sufficient.

For Linux DT I tried the DT overlay which seems to work fine.  I'm
using it to fix up phy-connection-type based on serdes configuration
that is known to u-boot.  Of course it's possible to apply an
overlay for each specific plug-in card in a given slot too, but that's
not practical especially for mixes of board with many slots combined
with many types of cards.  I'm not sure this can be done automatically
in U-Boot such that the user doesn't have to do any manual configuration
when swapping cards either.

U-Boot doesn't have the luxury of having overlays applied before it
starts, we don't use SPL.  I'm considering ways to have platform init
code apply some sort of fix-up or overlay for U-Boot DT based on serdes
configuration, but I don't have something running yet.  Live tree is
something I want to look into to see if it would work here.

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

They are not SFPs, they are normal MDIO driven PHYs just that they are
mounted on plug-in cards.


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

That is certainly inconsistent with the way other kinds of devices work.
What I did for DT based configuration on aquantia PHYs was to read the
DT properties in _config.  Just from a practical stand-point that's
fine, phydev->node is presumably set up by now and ethernet drivers have
to call _config anyway and that gives the PHY driver a chance to check
the DT properties.  But conceptually it is messy, it makes phy drivers
special.  I suppose the solution is to make phy drivers behave like
the other udevices, either actually making them udevices or at least
introducing phy_bind ahead of probe.

On the current code, do you see any issues with dealing with the
PHY DT properties in _config, other than this inconsistency with the
other devices?

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

This is a work-around for the previous issue I assume.

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

Are these switch ports or on a multi-port Ethernet card?  If that's the
case I would say the better solution is to actually drive ports as
ethernet devices in that case, rather than driving the switch as a whole
as an Ethernet interface.  I realize this approach may not work in all
cases, for various reasons, but then it's a matter of drivers that are
different not getting the benefit of using high level helper functions
that don't work for them.  At least we should make sure such drivers can
set things up properly if they are using custom binding or no DT at all.
On the switch topic I just sent some patches to support DSA switches
and the code drives ports as ethernet devices.  Perhaps the code is
not applicable to your device but I'd like to hear whether it makes
sense at the concept level.

> 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.
I think the nice way to fix that would be to introduce a bind function
for phy drivers, I could try to look into that after I free up my todo
list a little bit.

Thanks!
Alex