Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 12:06:24PM +0100, Russell King - ARM Linux wrote: > On Mon, Aug 28, 2017 at 11:40:51AM +0200, Antoine Tenart wrote: > > On Mon, Aug 28, 2017 at 09:51:52AM +0100, Russell King - ARM Linux wrote: > > > On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > > > > > > > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > > > > Alaska 88E1xxx series), is in-band link information always available? > > > > I'd be very cautious with such assumption and use in-band management > > > > only when set in the DT, like mvneta. I think phylib can properly can > > > > do its work when MDIO connection is provided on the board. > > > > > > There is another issue to be aware of: if you're wanting to use flow > > > control autonegotiation, that is not carried across SGMII's in-band > > > signalling. If you want to use SGMII's in-band signalling for the > > > duplex and speed information, you still need phylib's notification > > > to properly set the flow control. > > > > > > > > > Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on > > > mcbin) will handle all this for you - dealing with both in-band and > > > out-of-band negotiation methods, and combining them in the appropriate > > > manner for the selected operation mode. > > > > > > > So probably the best move here is to remove this patch, and wait for the > > phylink support in the PPv2 driver. > > I've nothing on that specifically for the mvpp2 driver - what I have is > for mvneta and the Marvell mvpp2x driver, with GMAC support extracted > from mvneta (that last bit is rather dirty at the moment so not > published anywhere, and doesn't cater for PP v2.1 at all.) > > I ought to have posted the mvneta part of the phylink patches, but I > didn't get around to it early enough in this cycle - there are probably > quite a number of conflicts with net-next now, so I think it's too late > to submit it for mainline. > > I know Andrew has already looked at them in my git tree as part of the > review of phylink when that was merged - which should be adequate to > give an example of how to implement it for the mainline PP v2 driver. OK, good. When looking at the phylink support in the PPv2 driver we'll look into what you did for mvneta (I saw it as well on your tree). In the meantime I'll respin this series without this patch. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 12:06:24PM +0100, Russell King - ARM Linux wrote: > On Mon, Aug 28, 2017 at 11:40:51AM +0200, Antoine Tenart wrote: > > On Mon, Aug 28, 2017 at 09:51:52AM +0100, Russell King - ARM Linux wrote: > > > On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > > > > > > > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > > > > Alaska 88E1xxx series), is in-band link information always available? > > > > I'd be very cautious with such assumption and use in-band management > > > > only when set in the DT, like mvneta. I think phylib can properly can > > > > do its work when MDIO connection is provided on the board. > > > > > > There is another issue to be aware of: if you're wanting to use flow > > > control autonegotiation, that is not carried across SGMII's in-band > > > signalling. If you want to use SGMII's in-band signalling for the > > > duplex and speed information, you still need phylib's notification > > > to properly set the flow control. > > > > > > > > > Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on > > > mcbin) will handle all this for you - dealing with both in-band and > > > out-of-band negotiation methods, and combining them in the appropriate > > > manner for the selected operation mode. > > > > > > > So probably the best move here is to remove this patch, and wait for the > > phylink support in the PPv2 driver. > > I've nothing on that specifically for the mvpp2 driver - what I have is > for mvneta and the Marvell mvpp2x driver, with GMAC support extracted > from mvneta (that last bit is rather dirty at the moment so not > published anywhere, and doesn't cater for PP v2.1 at all.) > > I ought to have posted the mvneta part of the phylink patches, but I > didn't get around to it early enough in this cycle - there are probably > quite a number of conflicts with net-next now, so I think it's too late > to submit it for mainline. > > I know Andrew has already looked at them in my git tree as part of the > review of phylink when that was merged - which should be adequate to > give an example of how to implement it for the mainline PP v2 driver. OK, good. When looking at the phylink support in the PPv2 driver we'll look into what you did for mvneta (I saw it as well on your tree). In the meantime I'll respin this series without this patch. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 11:40:51AM +0200, Antoine Tenart wrote: > On Mon, Aug 28, 2017 at 09:51:52AM +0100, Russell King - ARM Linux wrote: > > On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > > > > > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > > > Alaska 88E1xxx series), is in-band link information always available? > > > I'd be very cautious with such assumption and use in-band management > > > only when set in the DT, like mvneta. I think phylib can properly can > > > do its work when MDIO connection is provided on the board. > > > > There is another issue to be aware of: if you're wanting to use flow > > control autonegotiation, that is not carried across SGMII's in-band > > signalling. If you want to use SGMII's in-band signalling for the > > duplex and speed information, you still need phylib's notification > > to properly set the flow control. > > > > > > Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on > > mcbin) will handle all this for you - dealing with both in-band and > > out-of-band negotiation methods, and combining them in the appropriate > > manner for the selected operation mode. > > > > So probably the best move here is to remove this patch, and wait for the > phylink support in the PPv2 driver. I've nothing on that specifically for the mvpp2 driver - what I have is for mvneta and the Marvell mvpp2x driver, with GMAC support extracted from mvneta (that last bit is rather dirty at the moment so not published anywhere, and doesn't cater for PP v2.1 at all.) I ought to have posted the mvneta part of the phylink patches, but I didn't get around to it early enough in this cycle - there are probably quite a number of conflicts with net-next now, so I think it's too late to submit it for mainline. I know Andrew has already looked at them in my git tree as part of the review of phylink when that was merged - which should be adequate to give an example of how to implement it for the mainline PP v2 driver. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 11:40:51AM +0200, Antoine Tenart wrote: > On Mon, Aug 28, 2017 at 09:51:52AM +0100, Russell King - ARM Linux wrote: > > On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > > > > > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > > > Alaska 88E1xxx series), is in-band link information always available? > > > I'd be very cautious with such assumption and use in-band management > > > only when set in the DT, like mvneta. I think phylib can properly can > > > do its work when MDIO connection is provided on the board. > > > > There is another issue to be aware of: if you're wanting to use flow > > control autonegotiation, that is not carried across SGMII's in-band > > signalling. If you want to use SGMII's in-band signalling for the > > duplex and speed information, you still need phylib's notification > > to properly set the flow control. > > > > > > Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on > > mcbin) will handle all this for you - dealing with both in-band and > > out-of-band negotiation methods, and combining them in the appropriate > > manner for the selected operation mode. > > > > So probably the best move here is to remove this patch, and wait for the > phylink support in the PPv2 driver. I've nothing on that specifically for the mvpp2 driver - what I have is for mvneta and the Marvell mvpp2x driver, with GMAC support extracted from mvneta (that last bit is rather dirty at the moment so not published anywhere, and doesn't cater for PP v2.1 at all.) I ought to have posted the mvneta part of the phylink patches, but I didn't get around to it early enough in this cycle - there are probably quite a number of conflicts with net-next now, so I think it's too late to submit it for mainline. I know Andrew has already looked at them in my git tree as part of the review of phylink when that was merged - which should be adequate to give an example of how to implement it for the mainline PP v2 driver. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 09:51:52AM +0100, Russell King - ARM Linux wrote: > On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > > > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > > Alaska 88E1xxx series), is in-band link information always available? > > I'd be very cautious with such assumption and use in-band management > > only when set in the DT, like mvneta. I think phylib can properly can > > do its work when MDIO connection is provided on the board. > > There is another issue to be aware of: if you're wanting to use flow > control autonegotiation, that is not carried across SGMII's in-band > signalling. If you want to use SGMII's in-band signalling for the > duplex and speed information, you still need phylib's notification > to properly set the flow control. > > > Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on > mcbin) will handle all this for you - dealing with both in-band and > out-of-band negotiation methods, and combining them in the appropriate > manner for the selected operation mode. > So probably the best move here is to remove this patch, and wait for the phylink support in the PPv2 driver. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 09:51:52AM +0100, Russell King - ARM Linux wrote: > On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > > > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > > Alaska 88E1xxx series), is in-band link information always available? > > I'd be very cautious with such assumption and use in-band management > > only when set in the DT, like mvneta. I think phylib can properly can > > do its work when MDIO connection is provided on the board. > > There is another issue to be aware of: if you're wanting to use flow > control autonegotiation, that is not carried across SGMII's in-band > signalling. If you want to use SGMII's in-band signalling for the > duplex and speed information, you still need phylib's notification > to properly set the flow control. > > > Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on > mcbin) will handle all this for you - dealing with both in-band and > out-of-band negotiation methods, and combining them in the appropriate > manner for the selected operation mode. > So probably the best move here is to remove this patch, and wait for the phylink support in the PPv2 driver. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > Hi Antoine, > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > Alaska 88E1xxx series), is in-band link information always available? > I'd be very cautious with such assumption and use in-band management > only when set in the DT, like mvneta. I think phylib can properly can > do its work when MDIO connection is provided on the board. There is another issue to be aware of: if you're wanting to use flow control autonegotiation, that is not carried across SGMII's in-band signalling. If you want to use SGMII's in-band signalling for the duplex and speed information, you still need phylib's notification to properly set the flow control. Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on mcbin) will handle all this for you - dealing with both in-band and out-of-band negotiation methods, and combining them in the appropriate manner for the selected operation mode. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Mon, Aug 28, 2017 at 10:38:37AM +0200, Marcin Wojtas wrote: > Hi Antoine, > > Can you be 100% sure that when using SGMII with PHY's (like Marvell > Alaska 88E1xxx series), is in-band link information always available? > I'd be very cautious with such assumption and use in-band management > only when set in the DT, like mvneta. I think phylib can properly can > do its work when MDIO connection is provided on the board. There is another issue to be aware of: if you're wanting to use flow control autonegotiation, that is not carried across SGMII's in-band signalling. If you want to use SGMII's in-band signalling for the duplex and speed information, you still need phylib's notification to properly set the flow control. Switching mvpp2 to use phylink (which is needed for the 1G SFP slot on mcbin) will handle all this for you - dealing with both in-band and out-of-band negotiation methods, and combining them in the appropriate manner for the selected operation mode. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
Hi Antoine, 2017-08-28 8:55 GMT+02:00 Antoine Tenart: > Hi Russell, > > On Fri, Aug 25, 2017 at 11:43:13PM +0100, Russell King - ARM Linux wrote: >> On Fri, Aug 25, 2017 at 04:48:12PM +0200, Antoine Tenart wrote: >> > The link mode (speed, duplex) was forced based on what the phylib >> > returns. This should not be the case, and only forced by ethtool >> > functions manually. This patch removes the link mode enforcement from >> > the phylib link_event callback. >> >> So how does RGMII work (which has no in-band signalling between the PHY >> and MAC)? >> >> phylib expects the network driver to configure it according to the PHY >> state at link_event time - I think you need to explain more why you >> think that this is not necessary. > > Good catch, this won't work properly with RGMII. This could be done > out-of-band according to the spec, but that would use PHY polling and we > do not want that (the same concern was raised by Andrew on another > patch). > > I'll keep this mode enforcement for RGMII then. > Can you be 100% sure that when using SGMII with PHY's (like Marvell Alaska 88E1xxx series), is in-band link information always available? I'd be very cautious with such assumption and use in-band management only when set in the DT, like mvneta. I think phylib can properly can do its work when MDIO connection is provided on the board. Did you check the change also on A375? Best regards, Marcin
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
Hi Antoine, 2017-08-28 8:55 GMT+02:00 Antoine Tenart : > Hi Russell, > > On Fri, Aug 25, 2017 at 11:43:13PM +0100, Russell King - ARM Linux wrote: >> On Fri, Aug 25, 2017 at 04:48:12PM +0200, Antoine Tenart wrote: >> > The link mode (speed, duplex) was forced based on what the phylib >> > returns. This should not be the case, and only forced by ethtool >> > functions manually. This patch removes the link mode enforcement from >> > the phylib link_event callback. >> >> So how does RGMII work (which has no in-band signalling between the PHY >> and MAC)? >> >> phylib expects the network driver to configure it according to the PHY >> state at link_event time - I think you need to explain more why you >> think that this is not necessary. > > Good catch, this won't work properly with RGMII. This could be done > out-of-band according to the spec, but that would use PHY polling and we > do not want that (the same concern was raised by Andrew on another > patch). > > I'll keep this mode enforcement for RGMII then. > Can you be 100% sure that when using SGMII with PHY's (like Marvell Alaska 88E1xxx series), is in-band link information always available? I'd be very cautious with such assumption and use in-band management only when set in the DT, like mvneta. I think phylib can properly can do its work when MDIO connection is provided on the board. Did you check the change also on A375? Best regards, Marcin
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
Hi Russell, On Fri, Aug 25, 2017 at 11:43:13PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 25, 2017 at 04:48:12PM +0200, Antoine Tenart wrote: > > The link mode (speed, duplex) was forced based on what the phylib > > returns. This should not be the case, and only forced by ethtool > > functions manually. This patch removes the link mode enforcement from > > the phylib link_event callback. > > So how does RGMII work (which has no in-band signalling between the PHY > and MAC)? > > phylib expects the network driver to configure it according to the PHY > state at link_event time - I think you need to explain more why you > think that this is not necessary. Good catch, this won't work properly with RGMII. This could be done out-of-band according to the spec, but that would use PHY polling and we do not want that (the same concern was raised by Andrew on another patch). I'll keep this mode enforcement for RGMII then. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
Hi Russell, On Fri, Aug 25, 2017 at 11:43:13PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 25, 2017 at 04:48:12PM +0200, Antoine Tenart wrote: > > The link mode (speed, duplex) was forced based on what the phylib > > returns. This should not be the case, and only forced by ethtool > > functions manually. This patch removes the link mode enforcement from > > the phylib link_event callback. > > So how does RGMII work (which has no in-band signalling between the PHY > and MAC)? > > phylib expects the network driver to configure it according to the PHY > state at link_event time - I think you need to explain more why you > think that this is not necessary. Good catch, this won't work properly with RGMII. This could be done out-of-band according to the spec, but that would use PHY polling and we do not want that (the same concern was raised by Andrew on another patch). I'll keep this mode enforcement for RGMII then. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Fri, Aug 25, 2017 at 04:48:12PM +0200, Antoine Tenart wrote: > The link mode (speed, duplex) was forced based on what the phylib > returns. This should not be the case, and only forced by ethtool > functions manually. This patch removes the link mode enforcement from > the phylib link_event callback. So how does RGMII work (which has no in-band signalling between the PHY and MAC)? phylib expects the network driver to configure it according to the PHY state at link_event time - I think you need to explain more why you think that this is not necessary. > > Signed-off-by: Antoine Tenart> --- > drivers/net/ethernet/marvell/mvpp2.c | 24 > 1 file changed, 24 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvpp2.c > b/drivers/net/ethernet/marvell/mvpp2.c > index fab231858a41..498a4969dc58 100644 > --- a/drivers/net/ethernet/marvell/mvpp2.c > +++ b/drivers/net/ethernet/marvell/mvpp2.c > @@ -5741,30 +5741,10 @@ static void mvpp2_link_event(struct net_device *dev) > struct mvpp2_port *port = netdev_priv(dev); > struct phy_device *phydev = dev->phydev; > int status_change = 0; > - u32 val; > > if (phydev->link) { > if ((port->speed != phydev->speed) || > (port->duplex != phydev->duplex)) { > - u32 val; > - > - val = readl(port->base + MVPP2_GMAC_AUTONEG_CONFIG); > - val &= ~(MVPP2_GMAC_CONFIG_MII_SPEED | > - MVPP2_GMAC_CONFIG_GMII_SPEED | > - MVPP2_GMAC_CONFIG_FULL_DUPLEX | > - MVPP2_GMAC_AN_SPEED_EN | > - MVPP2_GMAC_AN_DUPLEX_EN); > - > - if (phydev->duplex) > - val |= MVPP2_GMAC_CONFIG_FULL_DUPLEX; > - > - if (phydev->speed == SPEED_1000) > - val |= MVPP2_GMAC_CONFIG_GMII_SPEED; > - else if (phydev->speed == SPEED_100) > - val |= MVPP2_GMAC_CONFIG_MII_SPEED; > - > - writel(val, port->base + MVPP2_GMAC_AUTONEG_CONFIG); > - > port->duplex = phydev->duplex; > port->speed = phydev->speed; > } > @@ -5782,10 +5762,6 @@ static void mvpp2_link_event(struct net_device *dev) > > if (status_change) { > if (phydev->link) { > - val = readl(port->base + MVPP2_GMAC_AUTONEG_CONFIG); > - val |= (MVPP2_GMAC_FORCE_LINK_PASS | > - MVPP2_GMAC_FORCE_LINK_DOWN); > - writel(val, port->base + MVPP2_GMAC_AUTONEG_CONFIG); > mvpp2_egress_enable(port); > mvpp2_ingress_enable(port); > } else { > -- > 2.13.5 > -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH net-next v2 05/14] net: mvpp2: do not force the link mode
On Fri, Aug 25, 2017 at 04:48:12PM +0200, Antoine Tenart wrote: > The link mode (speed, duplex) was forced based on what the phylib > returns. This should not be the case, and only forced by ethtool > functions manually. This patch removes the link mode enforcement from > the phylib link_event callback. So how does RGMII work (which has no in-band signalling between the PHY and MAC)? phylib expects the network driver to configure it according to the PHY state at link_event time - I think you need to explain more why you think that this is not necessary. > > Signed-off-by: Antoine Tenart > --- > drivers/net/ethernet/marvell/mvpp2.c | 24 > 1 file changed, 24 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvpp2.c > b/drivers/net/ethernet/marvell/mvpp2.c > index fab231858a41..498a4969dc58 100644 > --- a/drivers/net/ethernet/marvell/mvpp2.c > +++ b/drivers/net/ethernet/marvell/mvpp2.c > @@ -5741,30 +5741,10 @@ static void mvpp2_link_event(struct net_device *dev) > struct mvpp2_port *port = netdev_priv(dev); > struct phy_device *phydev = dev->phydev; > int status_change = 0; > - u32 val; > > if (phydev->link) { > if ((port->speed != phydev->speed) || > (port->duplex != phydev->duplex)) { > - u32 val; > - > - val = readl(port->base + MVPP2_GMAC_AUTONEG_CONFIG); > - val &= ~(MVPP2_GMAC_CONFIG_MII_SPEED | > - MVPP2_GMAC_CONFIG_GMII_SPEED | > - MVPP2_GMAC_CONFIG_FULL_DUPLEX | > - MVPP2_GMAC_AN_SPEED_EN | > - MVPP2_GMAC_AN_DUPLEX_EN); > - > - if (phydev->duplex) > - val |= MVPP2_GMAC_CONFIG_FULL_DUPLEX; > - > - if (phydev->speed == SPEED_1000) > - val |= MVPP2_GMAC_CONFIG_GMII_SPEED; > - else if (phydev->speed == SPEED_100) > - val |= MVPP2_GMAC_CONFIG_MII_SPEED; > - > - writel(val, port->base + MVPP2_GMAC_AUTONEG_CONFIG); > - > port->duplex = phydev->duplex; > port->speed = phydev->speed; > } > @@ -5782,10 +5762,6 @@ static void mvpp2_link_event(struct net_device *dev) > > if (status_change) { > if (phydev->link) { > - val = readl(port->base + MVPP2_GMAC_AUTONEG_CONFIG); > - val |= (MVPP2_GMAC_FORCE_LINK_PASS | > - MVPP2_GMAC_FORCE_LINK_DOWN); > - writel(val, port->base + MVPP2_GMAC_AUTONEG_CONFIG); > mvpp2_egress_enable(port); > mvpp2_ingress_enable(port); > } else { > -- > 2.13.5 > -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up