On Wed, 12 Aug 2020 16:48:37 +0100 Russell King - ARM Linux admin <li...@armlinux.org.uk> wrote:
> On Wed, Aug 12, 2020 at 05:37:16PM +0200, Marek Behún wrote: > > On Wed, 12 Aug 2020 16:00:54 +0100 > > Russell King - ARM Linux admin <li...@armlinux.org.uk> wrote: > > > > > On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote: > > > > There is another problem though: I think the PHY driver, when > > > > deciding whether to set MACTYPE from the XFI with rate matching > > > > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode, > > > > should check which modes the underlying MAC support. > > > > > > I'm aware of that problem. I have some experimental patches > > > which add PHY interface mode bitmaps to the MAC, PHY, and SFP > > > module parsing functions. I have stumbled on some problems > > > though - it's going to be another API change (and people are > > > already whinging about the phylink API changing "too quickly", > > > were too quickly seems to be defined as once in three years), and > > > in some cases, DSA, it's extremely hard to work out how to > > > properly set such a bitmap due to DSA's layered approach. > > > > > > > If by your experimental patches you mean > > net: mvneta: fill in phy interface mode bitmap > > net: mvpp2: fill in phy interface mode bitmap > > found here > > http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog > > I am currently working on top of them. > > > > > Having bitmaps means that we can take the union of what the MAC > > > and PHY supports, and decide which MACTYPE setting would be most > > > suitable. However, to do that we're into also changing phylib's > > > interfaces as well. > > > > > > > driver to phylink in the call to phylink_create. But there is > > > > no way for the PHY driver to get this information from phylink > > > > currently, and even if phylink exposed a function to return the > > > > config member of struct phylink, the problem is that at the > > > > time when mv3310_power_up is called, the phydev->phylink is not > > > > yet set (this is done in phylink_bringup_phy, and > > > > mv3310_power_up is called sometime in the phylink_attach_phy). > > > > > > > > > > We _really_ do not want phylib calling back into phylink > > > functions. That would tie phylink functionality into phylib and > > > cause problems when phylink is not being used. > > > > > > I would prefer phylib to be passed "the MAC can use these > > > interface types, and would prefer to use this interface type" and > > > have the phylib layer (along with the phylib driver) make the > > > decision about which mode should be used. That also means that > > > non-phylink MACs can also use it. > > > > > > > I may try to propose something, but in the meantime do you think the > > current version of the patch > > net: phy: marvell10g: change MACTYPE according to > > phydev->interface is acceptable? > > Well, I have other questions about it. Why are you doing it in > the power_up function? Do you find that the MACTYPE field is > lost when clearing the power down bit? From what I read, it should > only change on hardware reset, and we don't hardware reset when we > come out of power down - only software reset. > The MACTYPE is not being lost. But changing it requires Port Software Reset, which resets the link, so it cannot be done for example in read_status. I think the MACTYPE should be set sometime during PHY initialisation, and only once: either to XFI with rate matching, if the underlying MAC does not support lower modes, or to 10gbase-r/2500base-x/sgmii mode, if the underlying MAC supports only slower modes than 10G. Marek
