| Message ID | 20200810220645.19326-4-marek.behun@nic.cz |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Support for RollBall 10G copper SFP modules | expand |
On Tue, Aug 11, 2020 at 12:06:44AM +0200, Marek Behún wrote: > RollBall SFPs contain Marvell 88X3310 PHY, but they have configuration > pins strapped so that MACTYPE is configured in XFI with Rate Matching > mode. > > When these SFPs are inserted into a device which only supports lower > speeds on host interface, we need to configure the MACTYPE to a mode > in which the H unit changes SerDes speed according to speed on the > copper interface. I chose to use the > 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode. We actually need to have more inteligence in the driver, since we actually assume that it is in the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII mode without really checking. Note that there are differences in the way the mactype field is interpreted depending on exactly what chip we have. For example, 3310 and 3340 are different. That said, I've not heard of anyone using the 3340 yet.
On Tue, 11 Aug 2020 16:21:44 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Tue, Aug 11, 2020 at 12:06:44AM +0200, Marek Behún wrote: > > RollBall SFPs contain Marvell 88X3310 PHY, but they have > > configuration pins strapped so that MACTYPE is configured in XFI > > with Rate Matching mode. > > > > When these SFPs are inserted into a device which only supports lower > > speeds on host interface, we need to configure the MACTYPE to a mode > > in which the H unit changes SerDes speed according to speed on the > > copper interface. I chose to use the > > 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode. > > We actually need to have more inteligence in the driver, since we > actually assume that it is in the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII > mode without really checking. > > Note that there are differences in the way the mactype field is > interpreted depending on exactly what chip we have. For example, > 3310 and 3340 are different. That said, I've not heard of anyone > using the 3340 yet. > Russell, I am aware that MACTYPE modes are interpreted differently for 3310 vs 3340, but this only affects modes 0-3, which the driver does not check for even after applying my patch. 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. If the underlying MAC supports only XFI mode, than the MACTYPE should be set to XFI with rate matching. But on Omnia for example the MAC supports SGMII/1000base-s/2500base-x, so on Omnia the MACTYPE should be changed. Currently this information is given in your repository by the mvneta 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). Marek
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. 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.
On Wed, 12 Aug 2020 16:00:54 +0100 Russell King - ARM Linux admin <linux@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? Marek
> 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. Hi Russell If DSAs layering is causing real problems, we could rip it out, and let the driver directly interact with phylink. I'm not opposed to that. Andrew
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 <linux@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.
On Wed, Aug 12, 2020 at 05:44:36PM +0200, Andrew Lunn wrote: > > 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. > > Hi Russell > > If DSAs layering is causing real problems, we could rip it out, and > let the driver directly interact with phylink. I'm not opposed to > that. The reason I mentioned it is because I have this unpublished patch (beware, whitespace damaged due to copy-n-pasted): diff --git a/net/dsa/slave.c b/net/dsa/slave.c index c1967e08b017..ba32492f3ec0 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1629,6 +1629,12 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) dp->pl_config.dev = &slave_dev->dev; dp->pl_config.type = PHYLINK_NETDEV; + __set_bit(PHY_INTERFACE_MODE_SGMII, + dp->pl_config.supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_2500BASEX, + dp->pl_config.supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_1000BASEX, + dp->pl_config.supported_interfaces); /* The get_fixed_state callback takes precedence over polling the * link GPIO in PHYLINK (see phylink_get_fixed_state). Only set Which clearly is a gross hack - this code certainly has no idea about what interfaces the port itself supports. How do we get around that with DSA layering? We could add yet-another-driver-call down into the DSA driver for it to fill in that information and keep the current structure. However, is that really the best solution - to have lots of fine grained driver calls? DSA feels to be very cumbersome and awkward to modify at least to me. Almost everything seems to be "add another driver call" at the DSA layer, followed by "add another sub-driver call" at the mv88e6xxx layer.
On Wed, 12 Aug 2020 16:48:37 +0100 Russell King - ARM Linux admin <linux@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 <linux@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. > Changing MACTYPE requires Port Software Reset. I was not sure if this wouldn't break something. I am going to try to do this differently. Marek
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 <linux@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. That's fine, because you also have the patches further down in net-queue. So, what you list above are basically not all the patches. net: mvpp2: fill in phy interface mode bitmap net: mvneta: fill in phy interface mode bitmap net: phylink: use phy interface mode bitmaps net: sfp: add interface bitmap net: phy: add supported_interfaces to marvell10g PHYs net: phy: add supported_interfaces to marvell PHYs net: phy: add supported_interfaces to bcm84881 net: phy: add supported_interfaces to phylib Now, I think there's going to be a problem - if you look at the phylink code for this, we assume that the supported_interfaces we get from phylib is what the PHY will be doing. However, if we then go and change the interface mode, what then... I guess if we rework it along the lines that I've mentioned - phylib is passed the MAC supported interfaces, and phylib/phy drivers choose which interface to use, then I guess we no longer need to specify to phylib what interface mode should be used (what do we then do with the DT phy-mode property?) and we don't need some of the logic in "net: phylink: use phy interface mode bitmaps". Please note that it's been a number of months since I last looked at this, so I may be rusty - it's also rediculously sweltering hot over here in the UK right now, so I may not be thinking straight... (which is probably why I forgot to reply to your above point.)
On Wed, 12 Aug 2020 16:48:37 +0100 Russell King - ARM Linux admin <linux@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 <linux@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
On Wed, 12 Aug 2020 17:01:31 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > That's fine, because you also have the patches further down in > net-queue. So, what you list above are basically not all the patches. > > net: mvpp2: fill in phy interface mode bitmap > net: mvneta: fill in phy interface mode bitmap > net: phylink: use phy interface mode bitmaps > net: sfp: add interface bitmap > net: phy: add supported_interfaces to marvell10g PHYs > net: phy: add supported_interfaces to marvell PHYs > net: phy: add supported_interfaces to bcm84881 > net: phy: add supported_interfaces to phylib I did not list them all, but I applied them almost all your patches from clearfog branch to linus' master and am working on top of that
On Wed, Aug 12, 2020 at 06:13:33PM +0200, Marek Behún wrote: > 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. Wouldn't the right place to configure it be in the config_init() method - which is called once we have a MAC attaching to the PHY? As I mentioned, if we had a way to pass the MAC interface supported mask into phylib, config_init() could then use that to determine what to do. > 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. Yes - only changing the MAC type if we have good reason to do so to support other rates. There is a related problem however. Note that if you have an 88x3310 (non-P) in the SFP, then when rate matching is enabled, the PHY will _not_ generate pause frames, and the PHY expects the MAC to be configured to pace itself to the slower speed. I don't believe we have support in MACs for that, but phylib and therefore phylink provides the information: interface - 10GBASE-R speed - media speed pause - media pause modes So, if speed != SPEED_10000 and there are no pause modes, we should, for the sake of the entire link, pace the MAC to the media speed by controlling its egress rate.
On Wed, 12 Aug 2020 17:22:32 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Wed, Aug 12, 2020 at 06:13:33PM +0200, Marek Behún wrote: > > 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. > > Wouldn't the right place to configure it be in the config_init() > method - which is called once we have a MAC attaching to the PHY? > As I mentioned, if we had a way to pass the MAC interface supported > mask into phylib, config_init() could then use that to determine what > to do. > It is done from config_init. mv3310_power_up is called from mv3310_config_init. > > 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. > > Yes - only changing the MAC type if we have good reason to do so to > support other rates. > > There is a related problem however. Note that if you have an 88x3310 > (non-P) in the SFP, then when rate matching is enabled, the PHY will > _not_ generate pause frames, and the PHY expects the MAC to be > configured to pace itself to the slower speed. I don't believe we > have support in MACs for that, but phylib and therefore phylink > provides the information: > > interface - 10GBASE-R > speed - media speed > pause - media pause modes > > So, if speed != SPEED_10000 and there are no pause modes, we should, > for the sake of the entire link, pace the MAC to the media speed by > controlling its egress rate. >
On Wed, Aug 12, 2020 at 06:28:17PM +0200, Marek Behún wrote: > On Wed, 12 Aug 2020 17:22:32 +0100 > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > On Wed, Aug 12, 2020 at 06:13:33PM +0200, Marek Behún wrote: > > > 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. > > > > Wouldn't the right place to configure it be in the config_init() > > method - which is called once we have a MAC attaching to the PHY? > > As I mentioned, if we had a way to pass the MAC interface supported > > mask into phylib, config_init() could then use that to determine what > > to do. > > > > It is done from config_init. mv3310_power_up is called from > mv3310_config_init. I think it'll be best to resume this once we have saner weather.
On Wed, 12 Aug 2020 16:54:41 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Wed, Aug 12, 2020 at 05:44:36PM +0200, Andrew Lunn wrote: > > > 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. > > > > Hi Russell > > > > If DSAs layering is causing real problems, we could rip it out, and > > let the driver directly interact with phylink. I'm not opposed to > > that. > > The reason I mentioned it is because I have this unpublished patch > (beware, whitespace damaged due to copy-n-pasted): > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index c1967e08b017..ba32492f3ec0 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1629,6 +1629,12 @@ static int dsa_slave_phy_setup(struct > net_device *slave_dev) > > dp->pl_config.dev = &slave_dev->dev; > dp->pl_config.type = PHYLINK_NETDEV; > + __set_bit(PHY_INTERFACE_MODE_SGMII, > + dp->pl_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > + dp->pl_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_1000BASEX, > + dp->pl_config.supported_interfaces); > > /* The get_fixed_state callback takes precedence over polling > the > * link GPIO in PHYLINK (see phylink_get_fixed_state). Only > set > > Which clearly is a gross hack - this code certainly has no idea about > what interfaces the port itself supports. How do we get around that > with DSA layering? > > We could add yet-another-driver-call down into the DSA driver for it > to fill in that information and keep the current structure. However, > is that really the best solution - to have lots of fine grained driver > calls? > > DSA feels to be very cumbersome and awkward to modify at least to me. > Almost everything seems to be "add another driver call" at the DSA > layer, followed by "add another sub-driver call" at the mv88e6xxx > layer. > So I am encountering this now when testing my SFP + marvell10g patches on a board with SFP cage on a DSA port. I get what you find cumbersome, but I don't think there is other reasonable solution here. We already have .phylink_validate, which works with ethtool mode mask. So maybe a .phylink_config_init ? Marek
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 29575442b25b2..13a588fa69e77 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -81,6 +81,7 @@ enum { MV_V2_PORT_CTRL_SWRST = BIT(15), MV_V2_PORT_CTRL_PWRDOWN = BIT(11), MV_V2_PORT_MAC_TYPE_MASK = 0x7, + MV_V2_PORT_MAC_TYPE_10GBR_SGMII_AN = 0x4, MV_V2_PORT_MAC_TYPE_RATE_MATCH = 0x6, /* Temperature control/read registers (88X3310 only) */ MV_V2_TEMP_CTRL = 0xf08a, @@ -258,17 +259,40 @@ static int mv3310_power_down(struct phy_device *phydev) static int mv3310_power_up(struct phy_device *phydev) { struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); + u16 val, mask; int ret; ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, MV_V2_PORT_CTRL_PWRDOWN); - if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310 || - priv->firmware_ver < 0x00030000) + if (ret < 0) return ret; - return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, - MV_V2_PORT_CTRL_SWRST); + /* TODO: add support for changing MACTYPE on 88E2110 via register 1.C0A4.2:0 */ + if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310) + return 0; + + val = mask = MV_V2_PORT_CTRL_SWRST; + + switch (phydev->interface) { + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_2500BASEX: + val |= MV_V2_PORT_MAC_TYPE_10GBR_SGMII_AN; + mask |= MV_V2_PORT_MAC_TYPE_MASK; + break; + + default: + /* Otherwise we assume that the MACTYPE is set correctly by strapping pins. + * Feel free to add support for changing MACTYPE for other modes + * (XAUI/RXAUI/USXGMII). + */ + + /* reset is not needed for firmware version < 0.3.0.0 when not changing MACTYPE */ + if (priv->firmware_ver < 0x00030000) + return 0; + } + + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, mask, val); } static int mv3310_reset(struct phy_device *phydev, u32 unit)
RollBall SFPs contain Marvell 88X3310 PHY, but they have configuration pins strapped so that MACTYPE is configured in XFI with Rate Matching mode. When these SFPs are inserted into a device which only supports lower speeds on host interface, we need to configure the MACTYPE to a mode in which the H unit changes SerDes speed according to speed on the copper interface. I chose to use the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- drivers/net/phy/marvell10g.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)