Improvements
cover Improvements
Here comes a batch of patches for the via-rhine driver. Please apply.
via-rhine is still hardly usable on the most common Rhine hardware; it can't sustain 100Mbps traffic. The changes presented here improve the situation considerably; they fix a number of real problems and have been tested for regression (alas, by few people).
Roger
PATCH 1/4 Trivial changes; not affecting functionality
- nuke mii_status_bits
- move clear_tally_counters() up so it can get inlined (pointed out by Luca Barbieri)
- rename via_restart_tx() -> via_rhine_restart_tx()
drivers/net/via-rhine.c +19 -35
@@ -425,20 +425,6 @@ enum intr_status_bits { IntrNormalSummary=0x0003, IntrAbnormalSummary=0xC260, }; -/* MII interface, status flags. -Not to be confused with the MIIStatus register ... */ -enum mii_status_bits { - MIICap100T4 =0x8000 , - MIICap10100HdFd =0x7800 , - MIIPreambleSupr =0x0040 , - MIIAutoNegCompleted =0x0020 , - MIIRemoteFault =0x0010 , - MIICapAutoNeg =0x0008 , - MIILink =0x0004 , - MIIJabber =0x0002 , - MIIExtended =0x0001 -}; -/* The Rx and Tx buffer descriptors. */ struct rx_desc {s32 rx_status;
@@ -533,8 +519,6 @@ static void via_rhine_set_rx_mode(structstatic struct net_device_stats *via_rhine_get_stats (struct net_device *dev);static int netdev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);static int via_rhine_close (struct net_device *dev); -static inline void clear_tally_counters (long ioaddr); -static inline void via_restart_tx (struct net_device *dev);static void wait_for_reset (struct net_device *dev,int chip_id,char *name) {
@@ -797,7 +781,7 @@ static int __devinit via_rhine_init_one mdio_read(dev, phy, 5));/* set IFF_RUNNING */ -if (mii_status & MIILink) +if (mii_status &BMSR_LSTATUS )netif_carrier_on (dev);else netif_carrier_off (dev);
@@ -1175,8 +1159,8 @@ static void via_rhine_timer(unsigned lon/* make IFF_RUNNING follow the MII status bit "Link established" */ mii_status =mdio_read (dev, np->phys[0 ],MII_BMSR ); -if ( (mii_status & MIILink) != (np->mii_status & MIILink) ) { -if (mii_status & MIILink) +if ( (mii_status &BMSR_LSTATUS ) != (np->mii_status &BMSR_LSTATUS ) ) { +if (mii_status &BMSR_LSTATUS )netif_carrier_on (dev);else netif_carrier_off (dev);
@@ -1414,8 +1398,8 @@ static void via_rhine_rx(struct net_deviint boguscnt = np->dirty_rx +RX_RING_SIZE - np->cur_rx;if (debug >4 ) { -printk (KERN_DEBUG " In via_rhine_rx(), entry %d status %8.8x.\n" , - entry,le32_to_cpu (np->rx_head_desc->rx_status)); +printk (KERN_DEBUG "%s: via_rhine_rx(), entry %d status %8.8x.\n" , + dev->name, entry,le32_to_cpu (np->rx_head_desc->rx_status)); }/* If EOP is set on the next entry, it's a new packet. Send it up. */
@@ -1521,7 +1505,18 @@ static void via_rhine_rx(struct net_deviwritew (CmdRxDemand | np->chip_cmd, dev->base_addr + ChipCmd); } -static inline void via_restart_tx (struct net_device *dev) { +/* Clears the "tally counters" for CRC errors and missed frames(?). +It has been reported that some chips need a write of 0 to clear +these, for others the counters are set to 1 when written to and +instead cleared when read. So we clear them both ways ... */ +static inline void clear_tally_counters (const long ioaddr) +{ +writel (0 , ioaddr + RxMissed); +readw (ioaddr + RxCRCErrs); +readw (ioaddr + RxMissed); +} + +static inline void via_rhine_restart_tx (struct net_device *dev) {struct netdev_private *np = dev->priv;int entry = np->dirty_tx %TX_RING_SIZE ;
@@ -1561,7 +1556,7 @@ static void via_rhine_error(struct net_dif (debug >1 )printk (KERN_INFO "%s: Abort %4.4x, frame dropped.\n" , dev->name, intr_status); -via_restart_tx (dev); +via_rhine_restart_tx (dev); }if (intr_status & IntrTxUnderrun) {if (np->tx_thresh <0xE0 )
@@ -1570,7 +1565,7 @@ static void via_rhine_error(struct net_dprintk (KERN_INFO "%s: Transmitter underrun, Tx " "threshold now %2.2x.\n" , dev->name, np->tx_thresh); -via_restart_tx (dev); +via_rhine_restart_tx (dev); }if (intr_status & ~( IntrLinkChange | IntrStatsMax | IntrTxError | IntrTxAborted | IntrNormalSummary)) {
@@ -1599,17 +1594,6 @@ static struct net_device_stats *via_rhinreturn &np->stats; } -/* Clears the "tally counters" for CRC errors and missed frames(?). -It has been reported that some chips need a write of 0 to clear -these, for others the counters are set to 1 when written to and -instead cleared when read. So we clear them both ways ... */ -static inline void clear_tally_counters (const long ioaddr) -{ -writel (0 , ioaddr + RxMissed); -readw (ioaddr + RxCRCErrs); -readw (ioaddr + RxMissed); -} -static void via_rhine_set_rx_mode (struct net_device *dev) {struct netdev_private *np = dev->priv;
PATCH 2/4 Fix broken Tx underrun handling
In reply to Roger LuethiThe Tx underrun handling in the current driver is broken. The details have been explained in a posting "VIA Rhine 1.15exp1, patch for testing/discussion" a couple of months ago.
drivers/net/via-rhine.c +7 -7
@@ -416,9 +416,9 @@ int mmio_verify_registers[] = {/* Bits in the interrupt status/mask registers. */ enum intr_status_bits { IntrRxDone=0x0001 , IntrRxErr=0x0004 , IntrRxEmpty=0x0020 , - IntrTxDone=0x0002 , IntrTxError=0x0008 , IntrTxUnderrun=0x0010 , + IntrTxDone=0x0002 , IntrTxError=0x0008 , IntrTxUnderrun=0x0210 , IntrPCIErr=0x0040 , - IntrStatsMax=0x0080 , IntrRxEarly=0x0100 , IntrMIIChange=0x0200 , + IntrStatsMax=0x0080 , IntrRxEarly=0x0100 , IntrRxOverflow=0x0400 , IntrRxDropped=0x0800 , IntrRxNoBuf=0x1000 , IntrTxAborted=0x2000 , IntrLinkChange=0x4000 , IntrRxWakeUp=0x8000 ,
@@ -1005,7 +1005,7 @@ static void init_registers(struct net_dewritew (IntrRxDone | IntrRxErr | IntrRxEmpty| IntrRxOverflow | IntrRxDropped | IntrRxNoBuf | IntrTxAborted | IntrTxDone | IntrTxError | IntrTxUnderrun | - IntrPCIErr | IntrStatsMax | IntrLinkChange | IntrMIIChange, + IntrPCIErr | IntrStatsMax | IntrLinkChange, ioaddr + IntrEnable); np->chip_cmd = CmdStart|CmdTxOn|CmdRxOn|CmdNoTxPoll;
@@ -1310,7 +1310,7 @@ static void via_rhine_interrupt(int irq,via_rhine_tx (dev);/* Abnormal error summary/uncommon events handlers. */ -if (intr_status & (IntrPCIErr | IntrLinkChange | IntrMIIChange | +if (intr_status & (IntrPCIErr | IntrLinkChange | IntrStatsMax | IntrTxError | IntrTxAborted | IntrTxUnderrun))via_rhine_error (dev, intr_status);
@@ -1534,7 +1534,7 @@ static void via_rhine_error(struct net_dspin_lock (&np->lock); -if (intr_status & (IntrMIIChange | IntrLinkChange)) { +if (intr_status & (IntrLinkChange)) {if (readb (ioaddr + MIIStatus) &0x02 ) {/* Link failed, restart autonegotiation. */ if (np->drv_flags & HasDavicomPhy)
@@ -1552,7 +1552,7 @@ static void via_rhine_error(struct net_d np->stats.rx_missed_errors +=readw (ioaddr + RxMissed);clear_tally_counters (ioaddr); } -if (intr_status & IntrTxError) { +if (intr_status & IntrTxAborted) {if (debug >1 )printk (KERN_INFO "%s: Abort %4.4x, frame dropped.\n" , dev->name, intr_status);
@@ -1567,7 +1567,7 @@ static void via_rhine_error(struct net_d dev->name, np->tx_thresh);via_rhine_restart_tx (dev); } -if (intr_status & ~( IntrLinkChange | IntrStatsMax | +if (intr_status & ~( IntrLinkChange | IntrStatsMax | IntrTxUnderrun | IntrTxError | IntrTxAborted | IntrNormalSummary)) {if (debug >1 ) printk(KERN_ERR "%s: Something Wicked happened! %4.4x.\n",
PATCH 3/4 Various duplex related fixes
In reply to Roger LuethiFix the following bugs:
- after a watchdog timeout (still a common thing with the Rhine), the driver will fall back to half-duplex, ignoring the auto-negotiation results, killing performance completely until the driver is reloaded; fixed by clearing full_duplex in init_registers()
- driver considers only 0x200 for full-duplex option; now 0x220
- duplex code used dev->name before it's initialized; code section moved
The full_duplex and force_media related code is quite a mess, but this minimal fix will make the most annoying problems go away. Looking at some other code (e.g. mii.c) it seems to me the interaction between user force, autoneg results and full_duplex/force_media is amazingly complex, so I'll leave it at this for now.
drivers/net/via-rhine.c +18 -16
@@ -726,21 +726,6 @@ static int __devinit via_rhine_init_oneif (dev->mem_start) option = dev->mem_start; -/* The lower four bits are the media type. */ -if (option >0 ) { -if (option &0x200 ) - np->mii_if.full_duplex =1 ; - np->default_port = option &15 ; - } -if (card_idx <MAX_UNITS && full_duplex[card_idx] >0 ) - np->mii_if.full_duplex =1 ; - -if (np->mii_if.full_duplex) { -printk (KERN_INFO "%s: Set to forced full duplex, autonegotiation" -" disabled.\n" , dev->name); - np->mii_if.force_media =1 ; - } -/* The chip-specific entries in the device structure. */ dev->open = via_rhine_open; dev->hard_start_xmit = via_rhine_start_tx;
@@ -752,11 +737,27 @@ static int __devinit via_rhine_init_one dev->watchdog_timeo =TX_TIMEOUT ;if (np->drv_flags & ReqTxAlign) dev->features |=NETIF_F_SG |NETIF_F_HW_CSUM ; - + +/* dev->name not defined before register_netdev()! */ i =register_netdev (dev);if (i) goto err_out_unmap; +/* The lower four bits are the media type. */ +if (option >0 ) { +if (option &0x220 ) + np->mii_if.full_duplex =1 ; + np->default_port = option &15 ; + } +if (card_idx <MAX_UNITS && full_duplex[card_idx] >0 ) + np->mii_if.full_duplex =1 ; + +if (np->mii_if.full_duplex) { +printk (KERN_INFO "%s: Set to forced full duplex, autonegotiation" +" disabled.\n" , dev->name); + np->mii_if.force_media =1 ; + } +printk (KERN_INFO "%s: %s at 0x%lx, " , dev->name, via_rhine_chip_info[chip_id].name, (pci_flags &PCI_USES_IO ) ? ioaddr : memaddr);
@@ -992,6 +993,7 @@ static void init_registers(struct net_dewriteb (0x20 , ioaddr + TxConfig); np->tx_thresh =0x20 ; np->rx_thresh =0x60 ;/* Written in via_rhine_set_rx_mode(). */ + np->mii_if.full_duplex =0 ;if (dev->if_port ==0 ) dev->if_port = np->default_port;
PATCH 4/4 Reset function rewrite
In reply to Roger LuethiThe new reset function only waits up to 0.1 ms for the reset to complete, instead of 10 ms. However, it introduces the use of a forced reset flag if a chip doesn't seem cooperative. This fixes cases where until now only reloading the driver or even rebooting the machine would bring the chip back.
drivers/net/via-rhine.c +15 -14
@@ -523,24 +523,25 @@ static int via_rhine_close(struct net_dstatic void wait_for_reset (struct net_device *dev,int chip_id,char *name) {long ioaddr = dev->base_addr; -int i; -/* VT86C100A may need long delay after reset (dlink) */ -if (chip_id ==VT86C100A ) +udelay (5 ); + +if (readw (ioaddr + ChipCmd) & CmdReset) { +printk (KERN_INFO "%s: Reset did not complete in 5 us. " +"Trying harder.\n" , name); + +/* Rhine-II needs to be forced sometimes */ +if (chip_id ==VT6102 ) +writeb (0x40 , ioaddr +0x81 ); + +/* VT86C100A may need long delay after reset (dlink) */ +/* Seen on Rhine-II as well (rl) */ udelay (100 ); + } - i =0 ; -do { -udelay (5 ); - i++; -if (i >2000 ) { -printk (KERN_ERR "%s: reset did not complete in 10 ms.\n" , name); -break ; - } - }while (readw (ioaddr + ChipCmd) & CmdReset);if (debug >1 ) -printk (KERN_INFO "%s: reset finished after %d microseconds.\n" , - name,5 *i); +printk (KERN_INFO "%s: Reset %s.\n" , name, + (readw (ioaddr + ChipCmd) & CmdReset) ?"failed" :"succeeded" ); }#ifdef USE_MEM
Quoted 6 lines · Roger Luethi
Here comes a batch of patches for the via-rhine driver. Please apply.
via-rhine is still hardly usable on the most common Rhine hardware; it can't sustain 100Mbps traffic. The changes presented here improve the situation considerably; they fix a number of real problems and have been tested for regression (alas, by few people).
Looks good, all patches applied to 2.5.
Should these apply to 2.4, too?
Just a general comment, the reset logic seems a bit too much like voodoo magic ;-) It would be nice long-term to get an official answer from Via about the proper reset sequence and time limits. [regardless, like I said, patch applied...]
Jeff
Quoted 1 line · Jeff Garzik
Looks good, all patches applied to 2.5.
Thx. Patch to bump the version number up attached.
Quoted 1 line
Should these apply to 2.4, too?
Yes. I'm trying to keep the drivers in both trees in sync.
Quoted 2 lines
Just a general comment, the reset logic seems a bit too much like voodoo magic ;-)
Look at the rest of the driver and you will find the reset voodoo magic is a perfect fit. We'd be waving dead chickens if only tar could handle them. Actually, the reset logic is slightly better because for every line I have some reasoning and actual tests conducted. The underlying problem is that the Rhine is only loosely documented and various revisions differ in amazing ways from each other and from the documentation that supposedly describes them.
I've named the magic register in the patch below, does that ease your voodoo pain?
I've been considering for a while to document the driver somewhat better. Are documentation patches welcome, or do you just want to have official word from VIA that they agree with the reset code? And if I need a few lines to explain some voodoo magic, is the prefered way to put it into the source or would a freshly created Documentation/network/via-rhine.txt be (my first choice but the directory typically contains user rather than developer documentation) the better place?
Quoted 2 lines
It would be nice long-term to get an official answer from Via about the proper reset sequence and time limits. [regardless, like I
Heh. If I had a contact at VIA I'd have many and more important questions than the reset sequence that actually works now, unlike lots of other stuff. Yes, reliable information from within VIA -- official or not -- would be a big help.
Roger
drivers/net/via-rhine.c +11 -4
@@ -101,11 +101,18 @@LK1.1.15 (jgarzik): - Use new MII lib helper generic_mii_ioctl +LK1.1.16 (Roger Luethi) +- Etherleak fix +- Handle Tx buffer underrun +- Fix bugs in full duplex handling +- New reset code uses "force reset" cmd on Rhine-II +- Various clean ups +*/ #define DRV_NAME "via-rhine" -#define DRV_VERSION "1.1.15" -#define DRV_RELDATE "November-22-2002" +#define DRV_VERSION "1.1.16" +#define DRV_RELDATE "February-15-2003"/* A few user-configurable values.
@@ -395,7 +402,7 @@ enum register_offsets { MIIPhyAddr=0x6C , MIIStatus=0x6D , PCIBusConfig=0x6E , MIICmd=0x70 , MIIRegAddr=0x71 , MIIData=0x72 , MACRegEEcsr=0x74 , ConfigA=0x78 , ConfigB=0x79 , ConfigC=0x7A , ConfigD=0x7B , - RxMissed=0x7C , RxCRCErrs=0x7E , + RxMissed=0x7C , RxCRCErrs=0x7E , MiscCmd=0x81 , StickyHW=0x83, WOLcrClr=0xA4, WOLcgClr=0xA7, PwrcsrClr=0xAC, };
@@ -532,7 +539,7 @@ static void wait_for_reset(struct net_de/* Rhine-II needs to be forced sometimes */ if (chip_id ==VT6102 ) -writeb (0x40 , ioaddr +0x81 ); +writeb (0x40 , ioaddr + MiscCmd);/* VT86C100A may need long delay after reset (dlink) */ /* Seen on Rhine-II as well (rl) */
Jeff Garzik
AppliedQuoted 7 lines · Roger Luethi
Quoted 1 line · Jeff Garzik
Looks good, all patches applied to 2.5.
Thx. Patch to bump the version number up attached.
applied to 2.4 and 2.5
Quoted 4 lines
Quoted 1 line
Should these apply to 2.4, too?
Yes. I'm trying to keep the drivers in both trees in sync.
noted
Quoted 11 lines
Quoted 1 line
Just a general comment, the reset logic seems a bit too much like voodoo magic ;-)
Look at the rest of the driver and you will find the reset voodoo magic is a perfect fit. We'd be waving dead chickens if only tar could handle them. Actually, the reset logic is slightly better because for every line I have some reasoning and actual tests conducted. The underlying problem is that the Rhine is only loosely documented and various revisions differ in amazing ways from each other and from the documentation that supposedly describes them.
Quoted 2 lines
I've named the magic register in the patch below, does that ease your voodoo pain?
I applied the patch, but I meant more that wait_for_reset seems questionable. There is generally a PIO or MMIO write preceding wait_for_reset function call, and then the function delays. If the PCI write is posted, for example, which at least my own Via EPIA does, then you cannot be guaranteed the timing of
write[bwl]() udelay(5)
PCI writes must be flushed, by doing a read[bwl]().
So, obvious PCI posting bugs in the wait_for_reset may be the cause of some of the randomness that appears in the field. (Note this applies to all PCI writes in the driver...)
So, I said "voodoo magic" because it doesn't really seem like we know what the exact handling is... and randomly placed udelay() calls are, unfortunately, sometimes a sign of driver bugs instead of necessary hardware delays.
Quoted 7 lines
I've been considering for a while to document the driver somewhat better. Are documentation patches welcome, or do you just want to have official word from VIA that they agree with the reset code? And if I need a few lines to explain some voodoo magic, is the prefered way to put it into the source or would a freshly created
Documentation/network/via-rhine.txtbe (my first choice but the directory typically contains user rather than developer documentation) the better place?
I would prefer both user and developer docs go in Documentation/networking/via-rhine.txt. It is easy enough to note separate sections of the document...
Quoted 7 lines
Quoted 1 line
It would be nice long-term to get an official answer from Via about the proper reset sequence and time limits. [regardless, like I
Heh. If I had a contact at VIA I'd have many and more important questions than the reset sequence that actually works now, unlike lots of other stuff. Yes, reliable information from within VIA -- official or not -- would be a big help.
Let me wave some dead chickens of my own, and see what happens...
Jeff
Quoted 9 lines · Jeff Garzik
I applied the patch, but I meant more that
wait_for_resetseems questionable. There is generally a PIO or MMIO write precedingwait_for_resetfunction call, and then the function delays. If the PCI write is posted, for example, which at least my own Via EPIA does, then you cannot be guaranteed the timing ofwrite[bwl]() udelay(5)PCI writes must be flushed, by doing a read[bwl]().
Thanks for raising that issue. It is my understanding that PIO ops are synchronous (on IA-32). If that is correct, problems should only occur if the driver is built with MMIO support, no?
I have been building the driver without MMIO for quite a while to eliminate one source of problems for now.
Quoted 3 lines
what the exact handling is... and randomly placed
udelay()calls are, unfortunately, sometimes a sign of driver bugs instead of necessary hardware delays.
You won't see me rule out driver bugs as a likely explanation for anything anytime soon ;-).
Quoted 3 lines
I would prefer both user and developer docs go in
Documentation/networking/via-rhine.txt. It is easy enough to note separate sections of the document...
ACK.
Roger
Quoted 4 lines · Roger Luethi
Thanks for raising that issue. It is my understanding that PIO ops are synchronous (on IA-32). If that is correct, problems should only occur if the driver is built with MMIO support, no?
No, even PIO ops are asynchronous. They are _more_ synchronous than the MMIO ones (I think the CPU waits until they hit the bus, and most bridges just pass them through), but the CPU does not wait for them to hit the device.
So in practice, this _tends_ to mean that a PIO write will usually hit the device within a microsecond or less of being issued by the CPU, and you don't need a IO read to force it out. But considering the wide variety of PCI bridges out there I bet there are some that will post even PIO writes and might hold on to them for some time, especially if other activity like DMA keeps the bus busy.
In other words: I suspect the code will work. But it's probably _safer_ to do the normal "read to synchronize" unless there are major performance issues (which is clearly not the case in this particular instance, but might be somewhere else).
Linus
Quoted 8 lines · Roger Luethi
Quoted 4 lines
Thanks for raising that issue. It is my understanding that PIO ops are synchronous (on IA-32). If that is correct, problems should only occur if the driver is built with MMIO support, no?
No, even PIO ops are asynchronous. They are _more_ synchronous than the MMIO ones (I think the CPU waits until they hit the bus, and most bridges
Hmmm... A recent thread on PCI write posting seemed to confirm my view [1]. What am I missing here?
------------------------------ cut here -----------------------------------
From: Alan Cox <[email protected]>
Subject: Re: Linux 2.4.21-pre3-ac4
Date: 12 Jan 2003 20:40:54 +0000
Quoted 1 line · Benjamin Herrenschmidt
What about PCI write posting ? How can we enforce the 400ns delay here ?
For i/o space it is ok as in*/out* are synchronous. For mmio right now I don't know. I need to talk to Andre about that for SATA. I guess for the PPC its going to be fun
[...]
------------------------------ cut here -----------------------------------
Quoted 4 lines
don't need a IO read to force it out. But considering the wide variety of PCI bridges out there I bet there are some that will post even PIO writes and might hold on to them for some time, especially if other activity like DMA keeps the bus busy.
There was some talking about hwif->IOSYNC() (for IDE). That might be interesting for other devices, too. It could resolve to a nop for synchronous operations, and say a read* for MMIO. IMHO it shouldn't be up to a driver maintainer to figure out what sync op some arch the driver may run on needs. What a maintainer typically _can_ provide is type of operation (MMIO/PIO) and a register that is considered safe for a sync read.
Roger
[1] http://marc.theaimsgroup.com/?l=linux-kernel&m=104240180906935&w=4
Quoted 6 lines · Roger Luethi
Quoted 6 lines · Roger Luethi
Quoted 4 lines
Thanks for raising that issue. It is my understanding that PIO ops are synchronous (on IA-32). If that is correct, problems should only occur if the driver is built with MMIO support, no?
Quoted 6 lines
Quoted 2 lines
No, even PIO ops are asynchronous. They are _more_ synchronous than the MMIO ones (I think the CPU waits until they hit the bus, and most bridges
Hmmm... A recent thread on PCI write posting seemed to confirm my view [1]. What am I missing here?
Basically, in a wait_for_reset, performance doesn't matter, so just flush with a read, and it will work in all cases :)
A PIO write may be posted through multiple levels of PCI bridges, perhaps... in any case, even if PIO is completely synchronous in test cases, an additional read will not hurt here.
Jeff
Jeff Garzik
AppliedQuoted 6 lines · Roger Luethi
Here comes a batch of patches for the via-rhine driver. Please apply.
via-rhine is still hardly usable on the most common Rhine hardware; it can't sustain 100Mbps traffic. The changes presented here improve the situation considerably; they fix a number of real problems and have been tested for regression (alas, by few people).
applied to 2.4, as well.