Paul, The loopback mode forces internal full-duplex mode regardless of the fullduplex bit and hence it is ok to do this change.
Earlier in the history of the code this variable phy_duplex was a Boolean and got changed later to a integer value (should have been a enum/define as you suggested) and hence the bug that you found. I agree the implementation can be cleaner with defines/enum's. Regards, Anant -----Original Message----- From: Paul Bartholomew [mailto:[EMAIL PROTECTED] Sent: Thursday, December 07, 2006 6:57 PM To: Gole, Anant; [email protected] Subject: RE: BUGFIX for DaVinci ethernet driver (half-duplex/collisiondetection) >> But instead of changing line 4000 to check value of duplex, I suggest change line number 4006 >> if (dev->status.phy_duplex) { >> to >> if (dev->status.phy_duplex == 3) { Now that I've taken a closer look at the entire module, I suppose that would be a better fix. I based my mod on the comments where the "phy_duplex" element is defined ("1=full, 0=half"), and on the fact that "phy_duplex" was set to "1" a few lines above where I made my mod, and that later code (where you suggest changing) seems to treat it as a "boolean". Looking more closely, where the code was setting it to "1", I think the author *may* have meant "1 - Unknown" as opposed to "non-zero: full-duplex" (it's doing this when the emac is in 'loopback' mode, where I suppose the 'duplex' is unknown). But, does that mean that the "if" should be changed to: >> if ( (dev->status.phy_duplex == 3) || (dev->status.phy_duplex == 1) ) { Or, perhaps: >> if (dev->status.phy_duplex != 2) { ??? (I'm not sure how the EMAC_MACCONTROL_FULLDUPLEXEN bit should be set in 'loopback' mode) I feel that the original code would have been *much* clearer if it didn't use "1", "2", "3", etc. I think #define's or enum's should be used for this sort of thing. It would make it much clearer if the code was setting "phy_duplex" to either "DUPLEX_FULL" or "DUPLEX_UNKNOWN" instead of "1" (which is ambiguous, depending on whether the author was thinking of the var as a 'bool' vs thinking of it as an 'enum'). Overall, I think your suggested change is probably better - I'm just not sure what to do with the loopback case. - Paulb -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Gole, Anant Sent: Thursday, December 07, 2006 7:52 AM To: [email protected] Subject: RE: BUGFIX for DaVinci ethernet driver (half-duplex/collisiondetection) Paul, Good catch. The full duplex bit is getting set regardless causing issues when connected to a hub (half duplex). But instead of changing line 4000 to check value of duplex, I suggest change line number 4006 if (dev->status.phy_duplex) { to if (dev->status.phy_duplex == 3) { Since the variable phy_duplex is used in the function further for a debug log and it checks for the value of 3 for full duplex the above change will ensure that the debug log when enabled will print the right duplex value. Regards, Anant ---------------------------------------------------------------------- Date: Wed, 6 Dec 2006 12:04:25 -0000 From: "Paul Bartholomew" <[EMAIL PROTECTED]> Subject: BUGFIX for DaVinci ethernet driver (half-duplex/collision detection) To: [email protected] Message-ID: <[EMAIL PROTECTED]> Content-Type: text/plain; charset="iso-8859-1"; Hello - As pointed out in another thread ("Problems after updating to DVEVM 1.10" started by Andy ([EMAIL PROTECTED]), there seems to be problems connecting a DVEVM to an ethernet "hub". As I said in the other thread, I also had problems going through a 10/100 hub (half-duplex). There were MAJOR (several second) delays using the NFS-mounted root filesystem when going through a 'hub' (but bypassing the hub, it works fine). I've experimented more, and found that this combination fails (is slow): DVEVM -> HUB -> SWITCH -> PC But this one works: DVEVM -> SWITCH -> HUB -> SWITCH -> PC The only difference in these two scenarios is that the DVEVM would be talking 'half duplex' to the HUB in the first case, and talking 'full duplex' to the SWITCH in the second case (and that SWITCH would talk 'half duplex' to the HUB). If the HUB is introducing collisions/packet loss/etc, it would be the same number of errors in both cases. But, the speed is dramatically SLOWER when the DVEVM is directly connected to the HUB. As Andy pointed out in the earlier thread, the kernel that came with DVSDK "1.00" worked fine with a HUB, but the kernel with DVSDK "1.10" has this problem (I've confirmed that here). I've also confirmed that a recent 'git' kernel also has this slow-down with the HUB. I believe I've found the problem, and have a fix. The change is to "davinci_emac.c" in "driver/net". At line 3997 (function "emac_update_phy_status()"), the original code (from DVSDK "1.10" source) reads: if (set_phy_mode & SNWAY_LPBK) { dev->status.phy_duplex = 1; } else { dev->status.phy_duplex = emac_mdio_get_duplex(); } The problem is that the code in "davinci_emac.c" treats "dev->status.phy_duplex" as a 'boolean' : non-zero means full-duplex, and zero means half duplex. The function "emac_mdio_get_duplex()" returns an integer, with the following meanings: 0 - Auto-negotiate 1 - Unknown 2 - Half-duplex 3 - Full-duplex So, there are three possible non-zero return value, but only one of them ("3") means full-duplex. I've changed my copy of the code to this instead: if (set_phy_mode & SNWAY_LPBK) { dev->status.phy_duplex = 1; } else { dev->status.phy_duplex = ((emac_mdio_get_duplex() == 3) ? 1 : 0); } (Only set "dev->status.phy_duplex" to "1" if the emac_mdio_get_duplex() return value is "3" (full-duplex). Otherwise, assume half-duplex). Because collision detection/recovery is only enabled in the hardware in 'half duplex' mode, the 'bug' above meant that collisions were never detected (you could see that from "ifconfig" output). With the above 'fix', collisions are now detected/handled correctly. I've only done limited testing on this 'fix'. I'd appreciate it if someone else would give it a try/confirm my findings. I hope this helps others having the same problem. - Paulb _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
