Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
(quick reply...) On Fri, Jan 06, 2017 at 02:50:21PM +0100, Jerome Brunet wrote: > So I'm not sure I understand, are you against EEE integration in phylib > entirely, or specifically against the test I added in set_eee to filter > out broken modes ? I'm happy to see EEE integrated into phylib, but I think the current implementation is very buggy and needs a rewrite. > > BTW, one of the problems (not caused by your patch) is that changing > > the EEE advertisment does not (on all PHY drivers) cause the link to > > be renegotiated - there's no call to phy_start_aneg() when the advert > > changes, and even if there was, there's no guarantee that > > phy_start_aneg() will even set the AN restart bit in the control > > register. > > > > However, given that you're hooking into the set_eee function, I'm not > > sure why you placed your EEE advertisment thing into config_aneg() - > > isn't it more an initialisation thing (so should be in > > config_init()?) > > What I change is what the PHY advertise, so it seems logical to do it > where "genphy_config_advert" was called. Just taking the existing code > as an example You need to adjust the adverisment in two places: 1. On initialisation, when you need to change the default value. 2. Whenever the user requests a different EEE advertisment. You don't need to do it each time config_aneg() is called - nothing's going to change the EEE advertisment in that path. Hence, to check it each and every time seems like a waste of CPU cycles. However, there's another path that needs to be considered, which the current EEE code fails to do, and that is the resume path. Nothing at present saves and restores the EEE settings, they are completely lost if the PHY is powered down. This is just another symptom of the current poor quality EEE implementation in phylib, and another reason why I say above that the EEE code is in need of a rewrite... which is something I will be looking at. If the EEE settings are properly saved and restored over suspend/ resume, then the previously programmed EEE advertisment would also be restored. -- 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.
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
(quick reply...) On Fri, Jan 06, 2017 at 02:50:21PM +0100, Jerome Brunet wrote: > So I'm not sure I understand, are you against EEE integration in phylib > entirely, or specifically against the test I added in set_eee to filter > out broken modes ? I'm happy to see EEE integrated into phylib, but I think the current implementation is very buggy and needs a rewrite. > > BTW, one of the problems (not caused by your patch) is that changing > > the EEE advertisment does not (on all PHY drivers) cause the link to > > be renegotiated - there's no call to phy_start_aneg() when the advert > > changes, and even if there was, there's no guarantee that > > phy_start_aneg() will even set the AN restart bit in the control > > register. > > > > However, given that you're hooking into the set_eee function, I'm not > > sure why you placed your EEE advertisment thing into config_aneg() - > > isn't it more an initialisation thing (so should be in > > config_init()?) > > What I change is what the PHY advertise, so it seems logical to do it > where "genphy_config_advert" was called. Just taking the existing code > as an example You need to adjust the adverisment in two places: 1. On initialisation, when you need to change the default value. 2. Whenever the user requests a different EEE advertisment. You don't need to do it each time config_aneg() is called - nothing's going to change the EEE advertisment in that path. Hence, to check it each and every time seems like a waste of CPU cycles. However, there's another path that needs to be considered, which the current EEE code fails to do, and that is the resume path. Nothing at present saves and restores the EEE settings, they are completely lost if the PHY is powered down. This is just another symptom of the current poor quality EEE implementation in phylib, and another reason why I say above that the EEE code is in need of a rewrite... which is something I will be looking at. If the EEE settings are properly saved and restored over suspend/ resume, then the previously programmed EEE advertisment would also be restored. -- 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.
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Fri, 2017-01-06 at 11:42 +, Russell King - ARM Linux wrote: > On Fri, Jan 06, 2017 at 11:11:36AM +0100, Jerome Brunet wrote: > > > > The purpose of this patch is to provide a way to mark as broken a > > particular eee mode. At first, it had nothing to do with "set_eee" > > but, > > as Florian rightly pointed out, users shouldn't be able to re- > > enable a > > broken mode. > > I think something else has been missed - I don't see much point to > telling userspace that (eg) 1000baseT EEE is supported and then > ignore attempts to advertise it. > > If it's broken, then arguably the hardware doesn't support the mode, > so we should really be masking those bits from the EEE supported mask > as well. indeed. > > > [...] > > > > > > > > > - maybe the problem here is that the PCS doesn't support support > > > EEE in 1000baseT mode? > > > > > > It does, and that's kind of the problem. EEE in ON for 100Tx and > > 1000T > > by default with this PHY. I have several platform with the same > > MAC-PHY > > combination. Only the OdroidC2 shows this particular issue with > > 1000T- > > EEE > > > > As explained in other mails in this thread. The problem does not > > come > > from the MAC entering LPI. It actually comes from the link partner > > entering LPI on the Rx path under significant Tx transfer. For some > > reason, this completely mess up our PHY. > > For a 1000baseT link to enter low power, both ends have to enter LPI > (see 802.3 78.1.3.3.1) - the Tx and Rx paths can't independently > enter > LPI. > > So, if you have a busy Tx link, the link itself can't be entering > LPI. > Your link partner may be sending a request to enter LPI due to its > own > Tx path being idle, which should then be forwarded to your MAC. > > It's pretty hard to see what could be messed up with that - I'd have > expected the problems to occur when both ends were idle and the link > had entered low power mode. Well, maybe I'm not explaining the issue very well. Here the test done which led me to this conclusion: The test are done using iperf. Receiving data works well, with the expected performance. Sending data is the problem, and only under high load: Here are the lpi stats before starting the test: irq_tx_path_in_lpi_mode_n: 6 irq_tx_path_exit_lpi_mode_n: 5 irq_rx_path_in_lpi_mode_n: 76 irq_rx_path_exit_lpi_mode_n: 75 phy_eee_wakeup_error_n: 0 Sending data with iperf usually works for little while (between 0 and 10s) # iperf3 -c 192.168.1.170 -p12345 Connecting to host 192.168.1.170, port 12345 local 192.168.1.30 port 54450 connected to 192.168.1.170 port 12345 Interval Transfer Bandwidth Retr Cwnd 0.00-1.00 sec 112 MBytes 938 Mbits/sec0409 KBytes 1.00-2.00 sec 112 MBytes 940 Mbits/sec0426 KBytes 2.00-3.00 sec 112 MBytes 939 Mbits/sec0426 KBytes 3.00-4.00 sec 112 MBytes 940 Mbits/sec0426 KBytes 4.00-5.00 sec 112 MBytes 940 Mbits/sec0426 KBytes 5.00-6.00 sec 112 MBytes 939 Mbits/sec0426 KBytes 6.00-7.00 sec 9.26 MBytes 77.6 Mbits/sec2 1.41 KBytes <=Issue 7.00-8.00 sec 0.00 Bytes 0.00 bits/sec1 1.41 KBytes 8.00-9.00 sec 0.00 Bytes 0.00 bits/sec0 1.41 KBytes ^C10.00-13.58 sec 0.00 Bytes 0.00 bits/sec1 1.41 KBytes - - - - - - - - - - - - - - - - - - - - - - - - - Interval Transfer Bandwidth Retr 0.00-13.58 sec 681 MBytes 421 Mbits/sec4 sender 0.00-13.58 sec 0.00 Bytes 0.00 bits/sec receiver iperf3: interrupt - the client has terminated iperf3 does not exit ant the link seems completely broken. We cannot send or receive until the interface is brought down then up again. Here are the LPI related stats after the test: irq_tx_path_in_lpi_mode_n: 48 irq_tx_path_exit_lpi_mode_n: 48 irq_rx_path_in_lpi_mode_n: 325 irq_rx_path_exit_lpi_mode_n: 325 phy_eee_wakeup_error_n: 0 This happens with : 1) Default configuration: EEE enabled on the MAC, PHY with reset settings (EEE advertised) 2) EEE disabled on the MAC, PHY still with reset settings (EEE advertised). In such case there is no irq_tx_path_*_lpi_mode interrupts at all but still a lot of irq_rx_path_*_lpi_mode interrupts. So even if the mac does not drive anything EEE related, there is still something happening between the PHY and the link partner regarding EEE. 3) Disabling EEE advertisement for 1000t: no irq_*_lpi_mode at all. The feature is not negotiated and the Tx works well. By the way, EEE work well for the 100tx on the same HW. > > > > > > > > > On the SolidRun boards, they're using AR8035, and have suffered > > > this > > > occasional link drop problem. What has been found is that it > > > seems > > > to > > > be to do with the timing parameters, and it seemed to only be > > > 1000bT > > > that was affected. I don't remember off hand
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Fri, 2017-01-06 at 11:42 +, Russell King - ARM Linux wrote: > On Fri, Jan 06, 2017 at 11:11:36AM +0100, Jerome Brunet wrote: > > > > The purpose of this patch is to provide a way to mark as broken a > > particular eee mode. At first, it had nothing to do with "set_eee" > > but, > > as Florian rightly pointed out, users shouldn't be able to re- > > enable a > > broken mode. > > I think something else has been missed - I don't see much point to > telling userspace that (eg) 1000baseT EEE is supported and then > ignore attempts to advertise it. > > If it's broken, then arguably the hardware doesn't support the mode, > so we should really be masking those bits from the EEE supported mask > as well. indeed. > > > [...] > > > > > > > > > - maybe the problem here is that the PCS doesn't support support > > > EEE in 1000baseT mode? > > > > > > It does, and that's kind of the problem. EEE in ON for 100Tx and > > 1000T > > by default with this PHY. I have several platform with the same > > MAC-PHY > > combination. Only the OdroidC2 shows this particular issue with > > 1000T- > > EEE > > > > As explained in other mails in this thread. The problem does not > > come > > from the MAC entering LPI. It actually comes from the link partner > > entering LPI on the Rx path under significant Tx transfer. For some > > reason, this completely mess up our PHY. > > For a 1000baseT link to enter low power, both ends have to enter LPI > (see 802.3 78.1.3.3.1) - the Tx and Rx paths can't independently > enter > LPI. > > So, if you have a busy Tx link, the link itself can't be entering > LPI. > Your link partner may be sending a request to enter LPI due to its > own > Tx path being idle, which should then be forwarded to your MAC. > > It's pretty hard to see what could be messed up with that - I'd have > expected the problems to occur when both ends were idle and the link > had entered low power mode. Well, maybe I'm not explaining the issue very well. Here the test done which led me to this conclusion: The test are done using iperf. Receiving data works well, with the expected performance. Sending data is the problem, and only under high load: Here are the lpi stats before starting the test: irq_tx_path_in_lpi_mode_n: 6 irq_tx_path_exit_lpi_mode_n: 5 irq_rx_path_in_lpi_mode_n: 76 irq_rx_path_exit_lpi_mode_n: 75 phy_eee_wakeup_error_n: 0 Sending data with iperf usually works for little while (between 0 and 10s) # iperf3 -c 192.168.1.170 -p12345 Connecting to host 192.168.1.170, port 12345 local 192.168.1.30 port 54450 connected to 192.168.1.170 port 12345 Interval Transfer Bandwidth Retr Cwnd 0.00-1.00 sec 112 MBytes 938 Mbits/sec0409 KBytes 1.00-2.00 sec 112 MBytes 940 Mbits/sec0426 KBytes 2.00-3.00 sec 112 MBytes 939 Mbits/sec0426 KBytes 3.00-4.00 sec 112 MBytes 940 Mbits/sec0426 KBytes 4.00-5.00 sec 112 MBytes 940 Mbits/sec0426 KBytes 5.00-6.00 sec 112 MBytes 939 Mbits/sec0426 KBytes 6.00-7.00 sec 9.26 MBytes 77.6 Mbits/sec2 1.41 KBytes <=Issue 7.00-8.00 sec 0.00 Bytes 0.00 bits/sec1 1.41 KBytes 8.00-9.00 sec 0.00 Bytes 0.00 bits/sec0 1.41 KBytes ^C10.00-13.58 sec 0.00 Bytes 0.00 bits/sec1 1.41 KBytes - - - - - - - - - - - - - - - - - - - - - - - - - Interval Transfer Bandwidth Retr 0.00-13.58 sec 681 MBytes 421 Mbits/sec4 sender 0.00-13.58 sec 0.00 Bytes 0.00 bits/sec receiver iperf3: interrupt - the client has terminated iperf3 does not exit ant the link seems completely broken. We cannot send or receive until the interface is brought down then up again. Here are the LPI related stats after the test: irq_tx_path_in_lpi_mode_n: 48 irq_tx_path_exit_lpi_mode_n: 48 irq_rx_path_in_lpi_mode_n: 325 irq_rx_path_exit_lpi_mode_n: 325 phy_eee_wakeup_error_n: 0 This happens with : 1) Default configuration: EEE enabled on the MAC, PHY with reset settings (EEE advertised) 2) EEE disabled on the MAC, PHY still with reset settings (EEE advertised). In such case there is no irq_tx_path_*_lpi_mode interrupts at all but still a lot of irq_rx_path_*_lpi_mode interrupts. So even if the mac does not drive anything EEE related, there is still something happening between the PHY and the link partner regarding EEE. 3) Disabling EEE advertisement for 1000t: no irq_*_lpi_mode at all. The feature is not negotiated and the Tx works well. By the way, EEE work well for the 100tx on the same HW. > > > > > > > > > On the SolidRun boards, they're using AR8035, and have suffered > > > this > > > occasional link drop problem. What has been found is that it > > > seems > > > to > > > be to do with the timing parameters, and it seemed to only be > > > 1000bT > > > that was affected. I don't remember off hand
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Fri, Jan 06, 2017 at 06:42:24AM +0100, Yegor Yefremov wrote: > On Fri, Jan 6, 2017 at 12:25 AM, Russell King - ARM Linux >wrote: > > Another concern with this patch is that the existing phylib "set_eee" > > code is horribly buggy - it just translates the modes from userspace > > into the register value and writes them directly to the register with > > no validation. So it's possible to set modes in the register that the > > hardware doesn't support, and have them advertised to the link partner. > > > > I have a patch which fixes that, restricting (as we do elsewhere) the > > advert according to the EEE supported capabilities retrieved from the > > PCS - maybe the problem here is that the PCS doesn't support support > > EEE in 1000baseT mode? > > > > Out of interest, which PHY is used on this platform? > > > > On the SolidRun boards, they're using AR8035, and have suffered this > > occasional link drop problem. What has been found is that it seems to > > be to do with the timing parameters, and it seemed to only be 1000bT > > that was affected. I don't remember off hand exactly which or what > > the change was they made to stabilise it though, but I can probabily > > find out tomorrow. > > I have different boards with am335x and AR8035 and we had occasional > link drop with both 100 and 1000 speeds. AR8035 has "Smart EEE", which is a PHY specific thing... it's not entirely 802.3 compliant as it doesn't involve the MAC. The Smart EEE control registers are in the PCS MMD - some of this is from people's memories: - 0x805b is the TX wakeup timer. Lower 8 bits for 100base-Tx and upper 8 bits for 1000base-T. - 0x805c and 0x805d. The LPI timer is 24 bit, with the lower 16 bits in 0x805c and the upper 8 in 0x805d. 0x805d bit 8 appears to be the Smart EEE enable bit. What was found was setting the 1000base-T wakeup timer to the same as the 100base-Tx avoided the problems we were seeing, which was only with 1000base-T. (Whether that's because 100base-Tx hasn't been as well tested, I don't know.) SR ended up with 0x1717 in 0x805b. I'd suggest playing around with that register to see if extending the wakeup time has any beneficial effect. Also, I suspect Smart EEE shouldn't be enabled if you have an EEE capable MAC (in which case 0x805d bit 8 should be clear.) I think, however, it defaults to enabled. -- 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.
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Fri, Jan 06, 2017 at 06:42:24AM +0100, Yegor Yefremov wrote: > On Fri, Jan 6, 2017 at 12:25 AM, Russell King - ARM Linux > wrote: > > Another concern with this patch is that the existing phylib "set_eee" > > code is horribly buggy - it just translates the modes from userspace > > into the register value and writes them directly to the register with > > no validation. So it's possible to set modes in the register that the > > hardware doesn't support, and have them advertised to the link partner. > > > > I have a patch which fixes that, restricting (as we do elsewhere) the > > advert according to the EEE supported capabilities retrieved from the > > PCS - maybe the problem here is that the PCS doesn't support support > > EEE in 1000baseT mode? > > > > Out of interest, which PHY is used on this platform? > > > > On the SolidRun boards, they're using AR8035, and have suffered this > > occasional link drop problem. What has been found is that it seems to > > be to do with the timing parameters, and it seemed to only be 1000bT > > that was affected. I don't remember off hand exactly which or what > > the change was they made to stabilise it though, but I can probabily > > find out tomorrow. > > I have different boards with am335x and AR8035 and we had occasional > link drop with both 100 and 1000 speeds. AR8035 has "Smart EEE", which is a PHY specific thing... it's not entirely 802.3 compliant as it doesn't involve the MAC. The Smart EEE control registers are in the PCS MMD - some of this is from people's memories: - 0x805b is the TX wakeup timer. Lower 8 bits for 100base-Tx and upper 8 bits for 1000base-T. - 0x805c and 0x805d. The LPI timer is 24 bit, with the lower 16 bits in 0x805c and the upper 8 in 0x805d. 0x805d bit 8 appears to be the Smart EEE enable bit. What was found was setting the 1000base-T wakeup timer to the same as the 100base-Tx avoided the problems we were seeing, which was only with 1000base-T. (Whether that's because 100base-Tx hasn't been as well tested, I don't know.) SR ended up with 0x1717 in 0x805b. I'd suggest playing around with that register to see if extending the wakeup time has any beneficial effect. Also, I suspect Smart EEE shouldn't be enabled if you have an EEE capable MAC (in which case 0x805d bit 8 should be clear.) I think, however, it defaults to enabled. -- 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.
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Fri, Jan 06, 2017 at 11:11:36AM +0100, Jerome Brunet wrote: > The purpose of this patch is to provide a way to mark as broken a > particular eee mode. At first, it had nothing to do with "set_eee" but, > as Florian rightly pointed out, users shouldn't be able to re-enable a > broken mode. I think something else has been missed - I don't see much point to telling userspace that (eg) 1000baseT EEE is supported and then ignore attempts to advertise it. If it's broken, then arguably the hardware doesn't support the mode, so we should really be masking those bits from the EEE supported mask as well. > I wonder if we should return an error though. With the current > implementation, user space application could simply ask to activate > everything, excepting the kernel to sort it out and return an error > only if something horribly wrong happened. If we start returning an > error for unsupported modes, we could break things. I guess we should > just silently filter the requested modes. The ethtool behaviour for advertisments is that errors are not returned unless the attempted advert is really wrong. So, for example, when setting an advertisment for link modes, we accept the user's supplied mask, and bitwise AND it with the supported mask, so unsupported link modes are cleared. Only if the result is an empty mask do we then return an error to userspace. It's similar for forcing the link parameters - phylib attempts to find the best phy setting mode which fuzzily matches the users request, but doesn't error out if we can't do exactly what the user requested. In the EEE case, an empty mask is acceptable (it means "EEE is supported in no link modes") so it isn't appropriate to return errors there. > > - maybe the problem here is that the PCS doesn't support support > > EEE in 1000baseT mode? > > > It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T > by default with this PHY. I have several platform with the same MAC-PHY > combination. Only the OdroidC2 shows this particular issue with 1000T- > EEE > > As explained in other mails in this thread. The problem does not come > from the MAC entering LPI. It actually comes from the link partner > entering LPI on the Rx path under significant Tx transfer. For some > reason, this completely mess up our PHY. For a 1000baseT link to enter low power, both ends have to enter LPI (see 802.3 78.1.3.3.1) - the Tx and Rx paths can't independently enter LPI. So, if you have a busy Tx link, the link itself can't be entering LPI. Your link partner may be sending a request to enter LPI due to its own Tx path being idle, which should then be forwarded to your MAC. It's pretty hard to see what could be messed up with that - I'd have expected the problems to occur when both ends were idle and the link had entered low power mode. > > On the SolidRun boards, they're using AR8035, and have suffered this > > occasional link drop problem. What has been found is that it seems > > to > > be to do with the timing parameters, and it seemed to only be 1000bT > > that was affected. I don't remember off hand exactly which or what > > the change was they made to stabilise it though, but I can probabily > > find out tomorrow. > > > > Since the same combination of MAC-PHY works well on other designs, it > is also my feeling that is has something to do with some timing > parameter, maybe related to this particular PCB. Maybe a different PHY interface? Meson seems to use RGMII, maybe others use SGMII - but then I'd expect 100base-Tx to also be broken. So not really sure. I was talking to Florian about that last night, because the mis-named phy_init_eee() tests for various phy interface modes before proceeding, which seems to be fairly rubbish as the list of interface modes is gradually increasing since it was introduced (and I need to add SGMII to it.) The conclusion I've come to there is that the test should never have been part of phylib, because if there are restrictions on which phy interface modes are allowable for EEE, they're likely to be either PHY or MAC specific. The other problem that having the test there causes is that if the existing users can't handle EEE over SGMII, then when I add SGMII to support my hardware, they end up breaking - far from desirable. There's no information on why the test is there, or even which PHYs or MACs it's applicable to, which makes this unnecessarily more difficult to now resolve. My feeling is that the integration of EEE into phylib is fairly poor at the moment, and we need to be a lot smarter about it. BTW, one of the problems (not caused by your patch) is that changing the EEE advertisment does not (on all PHY drivers) cause the link to be renegotiated - there's no call to phy_start_aneg() when the advert changes, and even if there was, there's no guarantee that phy_start_aneg() will even set the AN restart bit in the control register. However, given that you're hooking into the set_eee function, I'm not
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Fri, Jan 06, 2017 at 11:11:36AM +0100, Jerome Brunet wrote: > The purpose of this patch is to provide a way to mark as broken a > particular eee mode. At first, it had nothing to do with "set_eee" but, > as Florian rightly pointed out, users shouldn't be able to re-enable a > broken mode. I think something else has been missed - I don't see much point to telling userspace that (eg) 1000baseT EEE is supported and then ignore attempts to advertise it. If it's broken, then arguably the hardware doesn't support the mode, so we should really be masking those bits from the EEE supported mask as well. > I wonder if we should return an error though. With the current > implementation, user space application could simply ask to activate > everything, excepting the kernel to sort it out and return an error > only if something horribly wrong happened. If we start returning an > error for unsupported modes, we could break things. I guess we should > just silently filter the requested modes. The ethtool behaviour for advertisments is that errors are not returned unless the attempted advert is really wrong. So, for example, when setting an advertisment for link modes, we accept the user's supplied mask, and bitwise AND it with the supported mask, so unsupported link modes are cleared. Only if the result is an empty mask do we then return an error to userspace. It's similar for forcing the link parameters - phylib attempts to find the best phy setting mode which fuzzily matches the users request, but doesn't error out if we can't do exactly what the user requested. In the EEE case, an empty mask is acceptable (it means "EEE is supported in no link modes") so it isn't appropriate to return errors there. > > - maybe the problem here is that the PCS doesn't support support > > EEE in 1000baseT mode? > > > It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T > by default with this PHY. I have several platform with the same MAC-PHY > combination. Only the OdroidC2 shows this particular issue with 1000T- > EEE > > As explained in other mails in this thread. The problem does not come > from the MAC entering LPI. It actually comes from the link partner > entering LPI on the Rx path under significant Tx transfer. For some > reason, this completely mess up our PHY. For a 1000baseT link to enter low power, both ends have to enter LPI (see 802.3 78.1.3.3.1) - the Tx and Rx paths can't independently enter LPI. So, if you have a busy Tx link, the link itself can't be entering LPI. Your link partner may be sending a request to enter LPI due to its own Tx path being idle, which should then be forwarded to your MAC. It's pretty hard to see what could be messed up with that - I'd have expected the problems to occur when both ends were idle and the link had entered low power mode. > > On the SolidRun boards, they're using AR8035, and have suffered this > > occasional link drop problem. What has been found is that it seems > > to > > be to do with the timing parameters, and it seemed to only be 1000bT > > that was affected. I don't remember off hand exactly which or what > > the change was they made to stabilise it though, but I can probabily > > find out tomorrow. > > > > Since the same combination of MAC-PHY works well on other designs, it > is also my feeling that is has something to do with some timing > parameter, maybe related to this particular PCB. Maybe a different PHY interface? Meson seems to use RGMII, maybe others use SGMII - but then I'd expect 100base-Tx to also be broken. So not really sure. I was talking to Florian about that last night, because the mis-named phy_init_eee() tests for various phy interface modes before proceeding, which seems to be fairly rubbish as the list of interface modes is gradually increasing since it was introduced (and I need to add SGMII to it.) The conclusion I've come to there is that the test should never have been part of phylib, because if there are restrictions on which phy interface modes are allowable for EEE, they're likely to be either PHY or MAC specific. The other problem that having the test there causes is that if the existing users can't handle EEE over SGMII, then when I add SGMII to support my hardware, they end up breaking - far from desirable. There's no information on why the test is there, or even which PHYs or MACs it's applicable to, which makes this unnecessarily more difficult to now resolve. My feeling is that the integration of EEE into phylib is fairly poor at the moment, and we need to be a lot smarter about it. BTW, one of the problems (not caused by your patch) is that changing the EEE advertisment does not (on all PHY drivers) cause the link to be renegotiated - there's no call to phy_start_aneg() when the advert changes, and even if there was, there's no guarantee that phy_start_aneg() will even set the AN restart bit in the control register. However, given that you're hooking into the set_eee function, I'm not
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Thu, 2017-01-05 at 23:25 +, Russell King - ARM Linux wrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > > > > If we start supporting generic "enable", "disable" type of > > properties > > with values that map directly to register definitions of the HW, we > > leave too much room for these properties to be utilized to > > implement a > > specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation. So it's possible to set modes in the register that > the > hardware doesn't support, and have them advertised to the link > partner. Hi Russell, The purpose of this patch is to provide a way to mark as broken a particular eee mode. At first, it had nothing to do with "set_eee" but, as Florian rightly pointed out, users shouldn't be able to re-enable a broken mode. At first, I was thinking about returning -ENOSUP if a broken mode was requested. Then I noticed the behavior you just described: A user can request anything of "set_eee", he won't necessarily get what he asked but won't get an error either. To avoid mixing different topic in a single patch, I kept the same behavior, not returning an error, just silently discarding broken modes I agree with you, we should probably validate a bit more what we asked of the hardware in set_eee. I wonder if we should return an error though. With the current implementation, user space application could simply ask to activate everything, excepting the kernel to sort it out and return an error only if something horribly wrong happened. If we start returning an error for unsupported modes, we could break things. I guess we should just silently filter the requested modes. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS Could be interesting :) > - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T by default with this PHY. I have several platform with the same MAC-PHY combination. Only the OdroidC2 shows this particular issue with 1000T- EEE As explained in other mails in this thread. The problem does not come from the MAC entering LPI. It actually comes from the link partner entering LPI on the Rx path under significant Tx transfer. For some reason, this completely mess up our PHY. > > Out of interest, which PHY is used on this platform? The PHY is the Realtek RTL8211F > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem. What has been found is that it seems > to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected. I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. > Since the same combination of MAC-PHY works well on other designs, it is also my feeling that is has something to do with some timing parameter, maybe related to this particular PCB. While debugging this issue, we tried to play with all the parameters we could think of but never found anything worth mentioning. If you have any ideas, I'd be happy to try. Jerome
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Thu, 2017-01-05 at 23:25 +, Russell King - ARM Linux wrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > > > > If we start supporting generic "enable", "disable" type of > > properties > > with values that map directly to register definitions of the HW, we > > leave too much room for these properties to be utilized to > > implement a > > specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation. So it's possible to set modes in the register that > the > hardware doesn't support, and have them advertised to the link > partner. Hi Russell, The purpose of this patch is to provide a way to mark as broken a particular eee mode. At first, it had nothing to do with "set_eee" but, as Florian rightly pointed out, users shouldn't be able to re-enable a broken mode. At first, I was thinking about returning -ENOSUP if a broken mode was requested. Then I noticed the behavior you just described: A user can request anything of "set_eee", he won't necessarily get what he asked but won't get an error either. To avoid mixing different topic in a single patch, I kept the same behavior, not returning an error, just silently discarding broken modes I agree with you, we should probably validate a bit more what we asked of the hardware in set_eee. I wonder if we should return an error though. With the current implementation, user space application could simply ask to activate everything, excepting the kernel to sort it out and return an error only if something horribly wrong happened. If we start returning an error for unsupported modes, we could break things. I guess we should just silently filter the requested modes. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS Could be interesting :) > - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T by default with this PHY. I have several platform with the same MAC-PHY combination. Only the OdroidC2 shows this particular issue with 1000T- EEE As explained in other mails in this thread. The problem does not come from the MAC entering LPI. It actually comes from the link partner entering LPI on the Rx path under significant Tx transfer. For some reason, this completely mess up our PHY. > > Out of interest, which PHY is used on this platform? The PHY is the Realtek RTL8211F > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem. What has been found is that it seems > to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected. I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. > Since the same combination of MAC-PHY works well on other designs, it is also my feeling that is has something to do with some timing parameter, maybe related to this particular PCB. While debugging this issue, we tried to play with all the parameters we could think of but never found anything worth mentioning. If you have any ideas, I'd be happy to try. Jerome
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
Hi Russel, On Fri, Jan 6, 2017 at 12:25 AM, Russell King - ARM Linuxwrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: >> If we start supporting generic "enable", "disable" type of properties >> with values that map directly to register definitions of the HW, we >> leave too much room for these properties to be utilized to implement a >> specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation. So it's possible to set modes in the register that the > hardware doesn't support, and have them advertised to the link partner. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? > > Out of interest, which PHY is used on this platform? > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem. What has been found is that it seems to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected. I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. I have different boards with am335x and AR8035 and we had occasional link drop with both 100 and 1000 speeds. Yegor
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
Hi Russel, On Fri, Jan 6, 2017 at 12:25 AM, Russell King - ARM Linux wrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: >> If we start supporting generic "enable", "disable" type of properties >> with values that map directly to register definitions of the HW, we >> leave too much room for these properties to be utilized to implement a >> specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation. So it's possible to set modes in the register that the > hardware doesn't support, and have them advertised to the link partner. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? > > Out of interest, which PHY is used on this platform? > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem. What has been found is that it seems to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected. I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. I have different boards with am335x and AR8035 and we had occasional link drop with both 100 and 1000 speeds. Yegor
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > If we start supporting generic "enable", "disable" type of properties > with values that map directly to register definitions of the HW, we > leave too much room for these properties to be utilized to implement a > specific policy, and this is not acceptable. Another concern with this patch is that the existing phylib "set_eee" code is horribly buggy - it just translates the modes from userspace into the register value and writes them directly to the register with no validation. So it's possible to set modes in the register that the hardware doesn't support, and have them advertised to the link partner. I have a patch which fixes that, restricting (as we do elsewhere) the advert according to the EEE supported capabilities retrieved from the PCS - maybe the problem here is that the PCS doesn't support support EEE in 1000baseT mode? Out of interest, which PHY is used on this platform? On the SolidRun boards, they're using AR8035, and have suffered this occasional link drop problem. What has been found is that it seems to be to do with the timing parameters, and it seemed to only be 1000bT that was affected. I don't remember off hand exactly which or what the change was they made to stabilise it though, but I can probabily find out tomorrow. -- 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.
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > If we start supporting generic "enable", "disable" type of properties > with values that map directly to register definitions of the HW, we > leave too much room for these properties to be utilized to implement a > specific policy, and this is not acceptable. Another concern with this patch is that the existing phylib "set_eee" code is horribly buggy - it just translates the modes from userspace into the register value and writes them directly to the register with no validation. So it's possible to set modes in the register that the hardware doesn't support, and have them advertised to the link partner. I have a patch which fixes that, restricting (as we do elsewhere) the advert according to the EEE supported capabilities retrieved from the PCS - maybe the problem here is that the PCS doesn't support support EEE in 1000baseT mode? Out of interest, which PHY is used on this platform? On the SolidRun boards, they're using AR8035, and have suffered this occasional link drop problem. What has been found is that it seems to be to do with the timing parameters, and it seemed to only be 1000bT that was affected. I don't remember off hand exactly which or what the change was they made to stabilise it though, but I can probabily find out tomorrow. -- 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.
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On 11/30/2016 01:47 AM, Jerome Brunet wrote: >> If we start supporting generic "enable", "disable" type of properties >> with values that map directly to register definitions of the HW, we >> leave too much room for these properties to be utilized to implement >> a >> specific policy, and this is not acceptable. > > Florian, > > I agree that DT should not be used to setup a policy, but to describe > what the HW is. > > I tried to implement it the way you suggested, using phy fixup, too see > what it looks like. > There is 2 places in the code that seems (remotely) linked to the > issue: > - meson8b_dwmac driver : if the mac, regardless of the board/platform, > could not tolerate to have EEE activated, it would make sense to have > the fixup here. It can provide a C callback for such case. > - realtek phy driver: philosophy is kind of the same > > To be clear, it is doable and it works that way, but I don't think > embedding this directly in the code is the right way to do it. It seems > we are hiding an information specific about the board inside a generic > driver. So there are a few things about that: - if we were not on ARM64, there would be possibly a remote chance of having some concept of a board file which would be where such a PHY fixup, or fixup of any kind would reside - having the PHY fixup in the PHY driver gated by both an exact match on the PHY OUI *and* the specific affected board makes it reasonably easy to locate it > > We have several amlogic's design with the same MAC, sometimes with the > same PHY, which have no problem with EEE at all. The issue is really > about the board design. OK, not a problem then: of_machine_is_compatible() should help you here? > > What I propose is not an enable/disable configuration switch, but to > clearly state that a particular mode of operation is broken. Like the > "max-speed" property, it setup a restriction. IMO, this is a > description of what the HW is and is capable of, and as such it should > be part of the DT. Sure, there is a fine line between describing what's broken, and being able to use that to actually configure non-broken hardware the way you want. > > Yes the property directly map to a register, but it does let you > directly manipulate it (you can't pass the value you want to write in > the register). Having it this way just makes the code simple on both > ends (user and driver). That's exactly the part that is giving me the creeps, any property that directly maps to a register value has a chance of a) leading to hard to debug problem if mis-configured, and b) being used as a policy as opposed to purely describing what is going on with the HW. > > Yes people could start abusing this to setup policy. In the end, it is > our responsibility, as community, to make sure APIs are used in a > proper way, and not let it be used that way. > > I'm open to suggestion on how improve the solution, maybe something > which could bring more confidence that property won't be misused. Once the binding lands in the kernel, there is absolutely zero guarantee nor visibility in how people end-up using in e.g: DT aware bootloader, and I am one of these people. Since there is a binding, there is consumer code in the kernel that needs to behave properly with respect to how the binding is defined. This is the same problem as with any kind of ABI, and a diverse range of consumers. -- Florian
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On 11/30/2016 01:47 AM, Jerome Brunet wrote: >> If we start supporting generic "enable", "disable" type of properties >> with values that map directly to register definitions of the HW, we >> leave too much room for these properties to be utilized to implement >> a >> specific policy, and this is not acceptable. > > Florian, > > I agree that DT should not be used to setup a policy, but to describe > what the HW is. > > I tried to implement it the way you suggested, using phy fixup, too see > what it looks like. > There is 2 places in the code that seems (remotely) linked to the > issue: > - meson8b_dwmac driver : if the mac, regardless of the board/platform, > could not tolerate to have EEE activated, it would make sense to have > the fixup here. It can provide a C callback for such case. > - realtek phy driver: philosophy is kind of the same > > To be clear, it is doable and it works that way, but I don't think > embedding this directly in the code is the right way to do it. It seems > we are hiding an information specific about the board inside a generic > driver. So there are a few things about that: - if we were not on ARM64, there would be possibly a remote chance of having some concept of a board file which would be where such a PHY fixup, or fixup of any kind would reside - having the PHY fixup in the PHY driver gated by both an exact match on the PHY OUI *and* the specific affected board makes it reasonably easy to locate it > > We have several amlogic's design with the same MAC, sometimes with the > same PHY, which have no problem with EEE at all. The issue is really > about the board design. OK, not a problem then: of_machine_is_compatible() should help you here? > > What I propose is not an enable/disable configuration switch, but to > clearly state that a particular mode of operation is broken. Like the > "max-speed" property, it setup a restriction. IMO, this is a > description of what the HW is and is capable of, and as such it should > be part of the DT. Sure, there is a fine line between describing what's broken, and being able to use that to actually configure non-broken hardware the way you want. > > Yes the property directly map to a register, but it does let you > directly manipulate it (you can't pass the value you want to write in > the register). Having it this way just makes the code simple on both > ends (user and driver). That's exactly the part that is giving me the creeps, any property that directly maps to a register value has a chance of a) leading to hard to debug problem if mis-configured, and b) being used as a policy as opposed to purely describing what is going on with the HW. > > Yes people could start abusing this to setup policy. In the end, it is > our responsibility, as community, to make sure APIs are used in a > proper way, and not let it be used that way. > > I'm open to suggestion on how improve the solution, maybe something > which could bring more confidence that property won't be misused. Once the binding lands in the kernel, there is absolutely zero guarantee nor visibility in how people end-up using in e.g: DT aware bootloader, and I am one of these people. Since there is a binding, there is consumer code in the kernel that needs to behave properly with respect to how the binding is defined. This is the same problem as with any kind of ABI, and a diverse range of consumers. -- Florian
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Mon, 2016-11-28 at 09:54 -0800, Florian Fainelli wrote: > On 11/28/2016 07:50 AM, Jerome Brunet wrote: > > > > This patchset fixes an issue with the OdroidC2 board (DWMAC + > > RTL8211F). > > The platform seems to enter LPI on the Rx path too often while > > performing > > relatively high TX transfer. This eventually break the link (both > > Tx and > > Rx), and require to bring the interface down and up again to get > > the Rx > > path working again. > > > > The root cause of this issue is not fully understood yet but > > disabling EEE > > advertisement on the PHY prevent this feature to be negotiated. > > With this change, the link is stable and reliable, with the > > expected > > throughput performance. > > > > The patchset adds options in the generic phy driver to disable EEE > > advertisement, through device tree. The way it is done is very > > similar > > to the handling of the max-speed property. > > > > Patch 4 is provided here for testing purpose only. Please don't > > merge > > patch 4, this change will go through the amlogic's tree. > > Sorry, but I really don't like the route this is going, and I should > have made myself clearer before on that, I really think utilizing a > PHY > fixup is more appropriate here than an extremely generic DT property. > The fixup code can be in the affected PHY driver, or it can be > somewhere > else, your call. There is no shortage of option on how to implement > it, > and this would be something easy to enable/disable for known good > configurations (ala PCI/USB fixups). > > If we start supporting generic "enable", "disable" type of properties > with values that map directly to register definitions of the HW, we > leave too much room for these properties to be utilized to implement > a > specific policy, and this is not acceptable. Florian, I agree that DT should not be used to setup a policy, but to describe what the HW is. I tried to implement it the way you suggested, using phy fixup, too see what it looks like. There is 2 places in the code that seems (remotely) linked to the issue: - meson8b_dwmac driver : if the mac, regardless of the board/platform, could not tolerate to have EEE activated, it would make sense to have the fixup here. It can provide a C callback for such case. - realtek phy driver: philosophy is kind of the same To be clear, it is doable and it works that way, but I don't think embedding this directly in the code is the right way to do it. It seems we are hiding an information specific about the board inside a generic driver. We have several amlogic's design with the same MAC, sometimes with the same PHY, which have no problem with EEE at all. The issue is really about the board design. What I propose is not an enable/disable configuration switch, but to clearly state that a particular mode of operation is broken. Like the "max-speed" property, it setup a restriction. IMO, this is a description of what the HW is and is capable of, and as such it should be part of the DT. Yes the property directly map to a register, but it does let you directly manipulate it (you can't pass the value you want to write in the register). Having it this way just makes the code simple on both ends (user and driver). Yes people could start abusing this to setup policy. In the end, it is our responsibility, as community, to make sure APIs are used in a proper way, and not let it be used that way. I'm open to suggestion on how improve the solution, maybe something which could bring more confidence that property won't be misused. Jerome
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On Mon, 2016-11-28 at 09:54 -0800, Florian Fainelli wrote: > On 11/28/2016 07:50 AM, Jerome Brunet wrote: > > > > This patchset fixes an issue with the OdroidC2 board (DWMAC + > > RTL8211F). > > The platform seems to enter LPI on the Rx path too often while > > performing > > relatively high TX transfer. This eventually break the link (both > > Tx and > > Rx), and require to bring the interface down and up again to get > > the Rx > > path working again. > > > > The root cause of this issue is not fully understood yet but > > disabling EEE > > advertisement on the PHY prevent this feature to be negotiated. > > With this change, the link is stable and reliable, with the > > expected > > throughput performance. > > > > The patchset adds options in the generic phy driver to disable EEE > > advertisement, through device tree. The way it is done is very > > similar > > to the handling of the max-speed property. > > > > Patch 4 is provided here for testing purpose only. Please don't > > merge > > patch 4, this change will go through the amlogic's tree. > > Sorry, but I really don't like the route this is going, and I should > have made myself clearer before on that, I really think utilizing a > PHY > fixup is more appropriate here than an extremely generic DT property. > The fixup code can be in the affected PHY driver, or it can be > somewhere > else, your call. There is no shortage of option on how to implement > it, > and this would be something easy to enable/disable for known good > configurations (ala PCI/USB fixups). > > If we start supporting generic "enable", "disable" type of properties > with values that map directly to register definitions of the HW, we > leave too much room for these properties to be utilized to implement > a > specific policy, and this is not acceptable. Florian, I agree that DT should not be used to setup a policy, but to describe what the HW is. I tried to implement it the way you suggested, using phy fixup, too see what it looks like. There is 2 places in the code that seems (remotely) linked to the issue: - meson8b_dwmac driver : if the mac, regardless of the board/platform, could not tolerate to have EEE activated, it would make sense to have the fixup here. It can provide a C callback for such case. - realtek phy driver: philosophy is kind of the same To be clear, it is doable and it works that way, but I don't think embedding this directly in the code is the right way to do it. It seems we are hiding an information specific about the board inside a generic driver. We have several amlogic's design with the same MAC, sometimes with the same PHY, which have no problem with EEE at all. The issue is really about the board design. What I propose is not an enable/disable configuration switch, but to clearly state that a particular mode of operation is broken. Like the "max-speed" property, it setup a restriction. IMO, this is a description of what the HW is and is capable of, and as such it should be part of the DT. Yes the property directly map to a register, but it does let you directly manipulate it (you can't pass the value you want to write in the register). Having it this way just makes the code simple on both ends (user and driver). Yes people could start abusing this to setup policy. In the end, it is our responsibility, as community, to make sure APIs are used in a proper way, and not let it be used that way. I'm open to suggestion on how improve the solution, maybe something which could bring more confidence that property won't be misused. Jerome
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On 11/28/2016 07:50 AM, Jerome Brunet wrote: > This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F). > The platform seems to enter LPI on the Rx path too often while performing > relatively high TX transfer. This eventually break the link (both Tx and > Rx), and require to bring the interface down and up again to get the Rx > path working again. > > The root cause of this issue is not fully understood yet but disabling EEE > advertisement on the PHY prevent this feature to be negotiated. > With this change, the link is stable and reliable, with the expected > throughput performance. > > The patchset adds options in the generic phy driver to disable EEE > advertisement, through device tree. The way it is done is very similar > to the handling of the max-speed property. > > Patch 4 is provided here for testing purpose only. Please don't merge > patch 4, this change will go through the amlogic's tree. Sorry, but I really don't like the route this is going, and I should have made myself clearer before on that, I really think utilizing a PHY fixup is more appropriate here than an extremely generic DT property. The fixup code can be in the affected PHY driver, or it can be somewhere else, your call. There is no shortage of option on how to implement it, and this would be something easy to enable/disable for known good configurations (ala PCI/USB fixups). If we start supporting generic "enable", "disable" type of properties with values that map directly to register definitions of the HW, we leave too much room for these properties to be utilized to implement a specific policy, and this is not acceptable. -- Florian
Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
On 11/28/2016 07:50 AM, Jerome Brunet wrote: > This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F). > The platform seems to enter LPI on the Rx path too often while performing > relatively high TX transfer. This eventually break the link (both Tx and > Rx), and require to bring the interface down and up again to get the Rx > path working again. > > The root cause of this issue is not fully understood yet but disabling EEE > advertisement on the PHY prevent this feature to be negotiated. > With this change, the link is stable and reliable, with the expected > throughput performance. > > The patchset adds options in the generic phy driver to disable EEE > advertisement, through device tree. The way it is done is very similar > to the handling of the max-speed property. > > Patch 4 is provided here for testing purpose only. Please don't merge > patch 4, this change will go through the amlogic's tree. Sorry, but I really don't like the route this is going, and I should have made myself clearer before on that, I really think utilizing a PHY fixup is more appropriate here than an extremely generic DT property. The fixup code can be in the affected PHY driver, or it can be somewhere else, your call. There is no shortage of option on how to implement it, and this would be something easy to enable/disable for known good configurations (ala PCI/USB fixups). If we start supporting generic "enable", "disable" type of properties with values that map directly to register definitions of the HW, we leave too much room for these properties to be utilized to implement a specific policy, and this is not acceptable. -- Florian