On Thu, Nov 18, 2021 at 05:33:33PM +0100, Andrew Lunn wrote: > > > + /* If supported is empty, just copy modes defined in fwnode. */ > > > + if (phy_interface_empty(supported)) > > > + return phy_interface_copy(supported, modes); > > > > Doesn't this mean we always end up with the supported_interfaces field > > filled in, even for drivers that haven't yet been converted? It will > > have the effect of locking the driver to the interface mode in "modes" > > where only one interface mode is mentioned in DT. > > > > At the moment, I think the only drivers that would be affected would be > > some DSA drivers, stmmac and macb as they haven't yet been converted. > > Hi Russell > > What do you think the best way forward is? Got those converted before > merging this? The situation is as follows: - For macb, I have a bunch of patches ready to go against net-next (in git log order): net: macb: use phylink_generic_validate() net: macb: clean up macb_validate() net: macb: remove interface checks in macb_validate() net: macb: populate supported_interfaces member but I think Sean and myself need to finish discussing PCS capabilities before I send those. - For mv88e6xxx DSA, I have some patches - I need to check with Marek whether he has any further changes for those as he's been looking over them and checking with the Marvell specs before I can send them. - For mt7530 DSA, I also have some patches, but no way to test them. All of the above patches are in my net-queue branch: http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue - For stmmac, I pinged Jose Abreu about it. stmmac's validate function does not check the interface mode at all if there is no XPCS attached. Jose says that which interface modes are supported is up to the integrator to decide, and it seems there is nothing in the current driver to work out which interface modes can be supported. If there is an XPCS attached, it defers interface mode checks to xpcs_validate(), which uses the interface mode to walk an array to find a list of linkmodes that the PCS supports. So, I think it's going to take a bit of unpicking to work out what to do here, and may depend on what we come up with with Sean for PCS. That leaves quite a number of DSA drivers untouched. So, I think we need to realise that even though we have all the users in the kernel, making changes to phylink's API is always going to be difficult, and we always need to maintain compatibility for old ways of doing stuff - at least until every user can be converted. However, that brings with it its own problem - there is then no motivation for people to adapt to the new way. This can be seen as we have the compatibility for calling mac_config() whenever the link comes up 16 months after the PCS stuff was introduced. One of the problem drivers for this is mtk_eth_soc: http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=278da006a936c0743c7fc261c23e7de992ac78e6 René van Dorst said in response to me posting that patch in July 2020: > I know, you have pointed that out before. But I don't know how to fix > mtk_gmac0_rgmii_adjust(). This function changes the PLL of the MAC. > But without documentation I am not sure what all the bits are used > for. and my attempts to discuss a way forward didn't get a reply. So, mtk_eth_soc has become an example of a problematical driver that makes ongoing phylink changes difficult, and I fear stmmac may become another example. I'm quite certain that as we try to develop phylink, such as adding extra facilities like specifying the interface modes, we're going to end up running into these kinds of problems that we can't solve, and we are going to have to keep compatibility for the old ways of doing stuff going for years to come - which is just going to get more and more painful. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
