* genphy_read_status() vs. 1000bT Pause capability @ 2017-03-28 0:49 Benjamin Herrenschmidt 2017-03-28 1:09 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 0:49 UTC (permalink / raw) To: netdev Hi ! I noticed that flow control isn't being enabled on a system I'm working on by default. I've tracked it down to two things: - The realtec.c PHY driver doesn't have Pause or Asym_Pause in its exposed capabilities. This is in part because PHY_GBIT_FEATURES does not include SUPPORTED_Pause and SUPPORTED_Asym_Pause. Is there a specific reason for that ? - After I've hacked the above, I get in genphy_read_status(): lpa=c1e1 lpagb=3800 adv=de1 common_adv=1e1 common_adv_gb=800 So we have negociated 1000bT full duplex. LPA_PAUSE's aren't set but I was under the impression that in Gigabit mode, LPA bit 0x80 which *is* set, meant ADVERTISE_1000XPAUSE which is the pause capability isn't it ? Or am I confusing with something else ? This seems to be how mii_adv_to_ethtool_adv_x() decodes them but that function is not called by genphy_read_status()... Now it's been a while since I hacked network drivers and back then everybody did their own salad with gigabit PHYs so it's very possible that I missed something here. Should we update genphy_read_status() to grab the pause details from mii_adv_to_ethtool_adv_x() when in 1000bT mode ? Thanks ! Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 0:49 genphy_read_status() vs. 1000bT Pause capability Benjamin Herrenschmidt @ 2017-03-28 1:09 ` Andrew Lunn 2017-03-28 2:28 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2017-03-28 1:09 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: netdev On Tue, Mar 28, 2017 at 11:49:50AM +1100, Benjamin Herrenschmidt wrote: > Hi ! > > I noticed that flow control isn't being enabled on a system I'm > working on by default. I've tracked it down to two things: > > - The realtec.c PHY driver doesn't have Pause or Asym_Pause in > its exposed capabilities. This is in part because PHY_GBIT_FEATURES > does not include SUPPORTED_Pause and SUPPORTED_Asym_Pause. Is there > a specific reason for that ? Hi Ben It is worth reading Documentation/networking/phy.txt The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC supports these features. The PHY will then negotiate them. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 1:09 ` Andrew Lunn @ 2017-03-28 2:28 ` Benjamin Herrenschmidt 2017-03-28 2:55 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 2:28 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev On Tue, 2017-03-28 at 03:09 +0200, Andrew Lunn wrote: > On Tue, Mar 28, 2017 at 11:49:50AM +1100, Benjamin Herrenschmidt wrote: > > Hi ! > > > > I noticed that flow control isn't being enabled on a system I'm > > working on by default. I've tracked it down to two things: > > > > - The realtec.c PHY driver doesn't have Pause or Asym_Pause in > > its exposed capabilities. This is in part because PHY_GBIT_FEATURES > > does not include SUPPORTED_Pause and SUPPORTED_Asym_Pause. Is there > > a specific reason for that ? > > Hi Ben > > It is worth reading Documentation/networking/phy.txt > > The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC > supports these features. The PHY will then negotiate them. Ok. I had added them but hit the other issue with the 1000bT style pause. Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 2:28 ` Benjamin Herrenschmidt @ 2017-03-28 2:55 ` Benjamin Herrenschmidt 2017-03-28 4:14 ` Florian Fainelli [not found] ` <20170328041405.045FEB206B@b01ledav03.gho.pok.ibm.com> 0 siblings, 2 replies; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 2:55 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev On Tue, 2017-03-28 at 13:28 +1100, Benjamin Herrenschmidt wrote: > > > Hi Ben > > > > It is worth reading Documentation/networking/phy.txt > > > > The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC > > supports these features. The PHY will then negotiate them. > Haha ! The OpenBMC kernel is still at 4.7 which was still saying you should only clear bits in there :-) I think that's what I initially read. Thanks for the pointer. Doesn't fix my other problem with Pause in 1000bT land. Do you know if that way of reflecting the pause capability by hijacking the old LPA bits is widely implemented enough that we should put it in genphy_read_status() ? Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 2:55 ` Benjamin Herrenschmidt @ 2017-03-28 4:14 ` Florian Fainelli [not found] ` <20170328041405.045FEB206B@b01ledav03.gho.pok.ibm.com> 1 sibling, 0 replies; 11+ messages in thread From: Florian Fainelli @ 2017-03-28 4:14 UTC (permalink / raw) To: benh, Andrew Lunn; +Cc: netdev, Russell King Hi Ben, On 03/27/2017 07:55 PM, Benjamin Herrenschmidt wrote: > On Tue, 2017-03-28 at 13:28 +1100, Benjamin Herrenschmidt wrote: >> >>> Hi Ben >>> >>> It is worth reading Documentation/networking/phy.txt >>> >>> The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC >>> supports these features. The PHY will then negotiate them. >> > Haha ! The OpenBMC kernel is still at 4.7 which was still saying you > should only clear bits in there :-) I think that's what I initially > read. > > Thanks for the pointer. > > Doesn't fix my other problem with Pause in 1000bT land. Do you know if > that way of reflecting the pause capability by hijacking the old > LPA bits is widely implemented enough that we should put it in > genphy_read_status() ? Not sure I follow you here? The link partner pause capability is reflected in phydev->pause and phydev->asym_pause (yes, these are terrible names) when the link is established. An Ethernet driver is still supposed to reconcile the locally advertised pause parameters (auto negotiated, or manually configured) from ethtool_{get,set}_param and then decide what to do in return (typically advertise or not the support for pause frames). The plan is eventually to provide better helper function for PHYLIB aware Ethernet MAC drivers such that given the local pause settings of the driver (resolved via ethtool), advertisement and auto-(re)negotation works as expected, essentially providing something generic ala tg3_set_pauseparam(). Have not gotten the time to get there yet so if you, or Russell beat me to it, I'd happily review such patches. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20170328041405.045FEB206B@b01ledav03.gho.pok.ibm.com>]
* Re: genphy_read_status() vs. 1000bT Pause capability [not found] ` <20170328041405.045FEB206B@b01ledav03.gho.pok.ibm.com> @ 2017-03-28 5:17 ` Benjamin Herrenschmidt 2017-03-28 5:19 ` Benjamin Herrenschmidt 2017-03-28 9:42 ` Russell King - ARM Linux 0 siblings, 2 replies; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 5:17 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn; +Cc: netdev, Russell King On Mon, 2017-03-27 at 21:14 -0700, Florian Fainelli wrote: > > Doesn't fix my other problem with Pause in 1000bT land. Do you know if > > that way of reflecting the pause capability by hijacking the old > > LPA bits is widely implemented enough that we should put it in > > genphy_read_status() ? > > Not sure I follow you here? The link partner pause capability is > reflected in phydev->pause and phydev->asym_pause (yes, these are > terrible names) when the link is established. Right. The problem is that they aren't for some gigabit links :-) Basically, in my setup with a PHY which uses genphy_read_status() (like most of them), I never get those advertised despite the fact that, I *think* they are supported by the other end (even after fixing my side of the advertising). I added a printk inside genphy_read_status() to inspect the result of the negociation, and this is what I read: lpa=c1e1 lpagb=3800 adv=de1 common_adv=1e1 common_adv_gb=800 As you can see, LPA doesn't have the Pause bits. *However* it does have bit 0x80 which can mean ADVERTISE_100HALF, but according to our own mii.h can also mean ADVERTISE_1000XPAUSE. Similarily it has bit 0x100 which can mean ADVERTISE_100FULL but also can mean ADVERTISE_1000XPSE_ASYM. In fact we appear to have two functions to interpret them as such inn the non-uapi mii.h: ethtool_adv_to_mii_adv_x mii_adv_to_ethtool_adv_x However they aren't used much in the tree and not at all by the "genphy" code. So my question is... when we observe that we have a 1000 link established, should we use these to "interpret" the LPA bits as above ? As it is, we never seem to advertise the capability because we never decode the above (tried with 2 different PHYs, a Realtek and a Broadcom) while my Cisco switches, I think, do support Pause. Hence my question ... how "standard" is the re-use of the LPA bits for these alternate meanings in 1000bT and should we update genphy to perform that decoding ? (I'm trying to download the 802.3 document referenced in the phy.txt to see if it says anything about it but it's taking forever for some reason). Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 5:17 ` Benjamin Herrenschmidt @ 2017-03-28 5:19 ` Benjamin Herrenschmidt 2017-03-28 9:42 ` Russell King - ARM Linux 1 sibling, 0 replies; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 5:19 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn; +Cc: netdev, Russell King On Tue, 2017-03-28 at 16:17 +1100, Benjamin Herrenschmidt wrote: > Hence my question ... how "standard" is the re-use of the LPA bits > for these alternate meanings in 1000bT and should we update genphy > to perform that decoding ? And btw, I'm happy to provide patches if we agree on the approach :-) Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 5:17 ` Benjamin Herrenschmidt 2017-03-28 5:19 ` Benjamin Herrenschmidt @ 2017-03-28 9:42 ` Russell King - ARM Linux 2017-03-28 11:11 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 11+ messages in thread From: Russell King - ARM Linux @ 2017-03-28 9:42 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Florian Fainelli, Andrew Lunn, netdev On Tue, Mar 28, 2017 at 04:17:18PM +1100, Benjamin Herrenschmidt wrote: > I added a printk inside genphy_read_status() to inspect the result > of the negociation, and this is what I read: > > lpa=c1e1 lpagb=3800 adv=de1 common_adv=1e1 common_adv_gb=800 > > As you can see, LPA doesn't have the Pause bits. *However* it does > have bit 0x80 which can mean ADVERTISE_100HALF, but according to > our own mii.h can also mean ADVERTISE_1000XPAUSE. Similarily it > has bit 0x100 which can mean ADVERTISE_100FULL but also can mean > ADVERTISE_1000XPSE_ASYM. The 1000X definitions are for 1000BaseX (fiber), not for 1000BaseT (copper). I have a working setup here with gigabit pause. The pause bits come from bits 11 and 10, just like for older copper PHYs. Your link parter is indicating that it has no pause capability. That means you can't use pause. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 9:42 ` Russell King - ARM Linux @ 2017-03-28 11:11 ` Benjamin Herrenschmidt 2017-03-28 11:31 ` Russell King - ARM Linux 0 siblings, 1 reply; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 11:11 UTC (permalink / raw) To: Russell King - ARM Linux; +Cc: Florian Fainelli, Andrew Lunn, netdev On Tue, 2017-03-28 at 10:42 +0100, Russell King - ARM Linux wrote: > The 1000X definitions are for 1000BaseX (fiber), not for 1000BaseT > (copper). > > I have a working setup here with gigabit pause. The pause bits come > from bits 11 and 10, just like for older copper PHYs. Your link > parter is indicating that it has no pause capability. That means > you can't use pause. Interesting. I tried and it worked :-) Could be something funny in the config of our switches. Thanks for the explanation. Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 11:11 ` Benjamin Herrenschmidt @ 2017-03-28 11:31 ` Russell King - ARM Linux 2017-03-28 20:32 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 11+ messages in thread From: Russell King - ARM Linux @ 2017-03-28 11:31 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Florian Fainelli, Andrew Lunn, netdev On Tue, Mar 28, 2017 at 10:11:24PM +1100, Benjamin Herrenschmidt wrote: > On Tue, 2017-03-28 at 10:42 +0100, Russell King - ARM Linux wrote: > > The 1000X definitions are for 1000BaseX (fiber), not for 1000BaseT > > (copper). > > > > I have a working setup here with gigabit pause. The pause bits come > > from bits 11 and 10, just like for older copper PHYs. Your link > > parter is indicating that it has no pause capability. That means > > you can't use pause. > > Interesting. I tried and it worked :-) Could be something funny in > the config of our switches. It could be that the switch supports pause frames, but it's not advertised because there's some corner cases that it doesn't work. I know that SolidRun have run into switches that corrupt ethernet frames when pause is used. (I don't know off hand which they are, but I could ask the question.) -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: genphy_read_status() vs. 1000bT Pause capability 2017-03-28 11:31 ` Russell King - ARM Linux @ 2017-03-28 20:32 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 11+ messages in thread From: Benjamin Herrenschmidt @ 2017-03-28 20:32 UTC (permalink / raw) To: Russell King - ARM Linux; +Cc: Florian Fainelli, Andrew Lunn, netdev On Tue, 2017-03-28 at 12:31 +0100, Russell King - ARM Linux wrote: > > Interesting. I tried and it worked :-) Could be something funny in > > the config of our switches. > > It could be that the switch supports pause frames, but it's not > advertised because there's some corner cases that it doesn't work. I > know that SolidRun have run into switches that corrupt ethernet frames > when pause is used. (I don't know off hand which they are, but I could > ask the question.) I think this is Cisco gear, I'll check with our lab guy today. In the meantime I'll test my driver back to back with some other machines, I should find something that does pause eventually :-) It's handy when you have a gigabit MAC on a 400Mhz ARM9 to be able to throttle the peer despite all the other problems with Pause :-) Cheers, Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-28 20:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-28 0:49 genphy_read_status() vs. 1000bT Pause capability Benjamin Herrenschmidt
2017-03-28 1:09 ` Andrew Lunn
2017-03-28 2:28 ` Benjamin Herrenschmidt
2017-03-28 2:55 ` Benjamin Herrenschmidt
2017-03-28 4:14 ` Florian Fainelli
[not found] ` <20170328041405.045FEB206B@b01ledav03.gho.pok.ibm.com>
2017-03-28 5:17 ` Benjamin Herrenschmidt
2017-03-28 5:19 ` Benjamin Herrenschmidt
2017-03-28 9:42 ` Russell King - ARM Linux
2017-03-28 11:11 ` Benjamin Herrenschmidt
2017-03-28 11:31 ` Russell King - ARM Linux
2017-03-28 20:32 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox