Date: Wed, 12 Dec 2007 12:41:14 +0900 From: Pyun YongHyeon <pyunyh@gmail.com> To: Alexander Pohoyda <alexander.pohoyda@gmx.net> Cc: freebsd-drivers@freebsd.org Subject: Re: SiS 190 NIC driver Message-ID: <20071212034114.GB68410@cdnetworks.co.kr> In-Reply-To: <20071211074509.281710@gmx.net> References: <200712102046.27504.alexander.pohoyda@gmx.net> <20071211012711.GB64299@cdnetworks.co.kr> <20071211074509.281710@gmx.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Dec 11, 2007 at 08:45:09AM +0100, Alexander Pohoyda wrote: > > > Attached is a driver for the SiS 190 NIC found in some Shuttle XPS > > > I've looked over the driver code and I guess the driver is not > > for RELENG_6/RELENG_7/CURRENT. Would you make it work for CURRENT? > > I would like to, but neither RELENG_6, nor RELENG_7 boots on my Shuttle box. I have reported this in -current mailing list some time ago, but got no usable suggestion. Maybe I should try a recent CURRENT, but I don't have much time to play around with it. Maybe sumbit a PR for this? > > > > - I guess SiS 190 is a gigabit controller but the probe message just > > shows 10/100TX message. Does this driver supports 1Gbps? > > I don't know for sure, but SiS website <http://www.sis.com/download/agreement.php?id=155880> mentions: SiS191 Gigabit LAN & SiS190 LAN Driver(v2.03) > > Other websites have it wrong, I guess, e.g.: <http://www.dirfile.com/sis_190_gigabit_lan__sis_191_lan_driver_v_1_03b.htm> SiS 190 Gigabit LAN & SiS 191 LAN Driver v 1.03b > > However, I will have a look at this. Thanks. > > > > - The usage of bus_dma(9) KPI seems to be incorrect. For example > > there should be no reason to set BUS_DMA_ALLOCNOW flag in > > bus_dma_tag_create. > > This code is left from the original sis driver and it works. Moreover, the Architecture Handbook <http://www.freebsd.org/doc/en_US.ISO8859-1/books/arch-handbook/isa-driver-busmem.html> does not mention bus_dmamap_load_mbuf_sg(9) and once again, newer FreeBSD releases do not start on this box at all, so if this new bus_dma API is not available in RELENG_5, I cannot use it. I don't think sis(4) is the best example for bus_dma(9) usage. If it was it would have worked on sparc64. Check otherdrivers that runs without problems on !i386 architecture. And yes, the Handbook needs updating. See bus_dma(9) man page. > > > > needed to pass/get to/from callback. Please check other network > > drivers for correct usage of bus_dma(9). > > The existing sis driver does no use bus_dmamap_load_mbuf_sg either. > > > > - It seems that Tx/Rx ring have alignment restrictions, probably > > 16 bytes or larger. Would you please check it? > > I do not understand, please elaborate. > For example, you seem to use alignemnet 1 in dma tags for Tx ring. 796 /* TX */ 797 error = bus_dma_tag_create(sc->sis_parent_tag, /* parent */ 798 1, 0, /* alignment, boundary */ ^^^ 799 BUS_SPACE_MAXADDR, /* lowaddr */ 800 BUS_SPACE_MAXADDR, /* highaddr */ 801 NULL, NULL, /* filter, filterarg */ 802 SIS_TX_RING_SZ, 1, /* maxsize,nsegments */ 803 BUS_SPACE_MAXSIZE_32BIT, /* maxsegsize */ 804 0, /* flags */ 805 busdma_lock_mutex, /* lockfunc */ 806 &Giant, /* lockarg */ 807 &ld->sis_tx_tag); I guess the the alignment would be sizeof(struct sis19x_desc) at least, i.e. 16 bytes. Normally ethernet controllers may have more larger alignment limitation than its single descriptor size(e.g. 256 bytes). You seems to use m_devget(9) for !386 to align recevied frames in sis_rxeof(). This clearly indicates it needs Rx buffer alignment limitation. In addition, you seem to use the same dma tag for Tx/Rx dma maps. Becasue most hardwares supports multi-fragmented frames in Tx side, total Tx DMA segment size would be larger than Rx DMA segment size. For Rx side, most hardwares just support 1 DMA segment, so it's maximum DMA segment size would be MCLBYTES. > > > - Descriptor counter 64 seems to be small for gigabit controllers. > > I guess 256 or higher would be more reasonable one. > > Do you mean that: > int sis_tx_cnt; > should rather be: > u_int256_t sis_tx_cnt; > > If not, please elaborate. > Sorry, I meant the following descriptor counters for Rx/Tx ring. 77 #define SIS_RX_RING_CNT 64 ^^^ 78 #define SIS_TX_RING_CNT 64 ^^^ > > > - I'm not sure SiS 190 can't handle DMA gathering in Tx path but > > it seems that Linux doesn't use multiple Tx descriptors at all > > in Tx path(NETIF_F_SG flag is absent in Linux) So I guess you can > > simplify sis_encap() function. > > Please offer your version of a function and I will test it. Or explain how you mean to simplify it. Is there an existing driver with a simplified variant? > Check vr(4)'s vr_encap, it simply calls m_defrag(9) as Rhine I hardware doesn't support multi-segment dma. > > > - It seems the driver name sis19x looks odd. How about sge(SiS > > Gigabit Ethernet)? > > Yes, I would like sge better, but see above. > > > > - Missing Makefile and man page. > > Sorry, cannot provide a man page other than a standard sis(4), but a Makefile is not a problem, will do. > > > > - style(9) cleanup. > > I tried to stick to style(9), do you have any specific complaints? > 198 Reserved0 = 0x08, // unused ^^^ 199 /* 2007-04-25, Alexander Pohoyda: this register is ^^^^ 200 * automatically set by NIC to the value of TxDescStartAddr 201 * register plus 8 (which, I suppose, is the offset of the 202 * sis_ptr field in sis19x_desc structure). */ ^^^ 203 TxNextDescAddr = 0x0c, // unused // comment, multi-line comment, missing () in return statement etc. > > > I don't have SiS 190 hardware to test the driver. So it's hard for me > > to make it work on CURRENT. Feel free to contact to me if you have > > any questions. > > Thank you very much for useful comments! Please send me code snippets and I will test them! I'm somewhat overloaded for other work so I'm afraid I can't write a code for not-having hardwares. It would be even better if you can check other drivers(bge, em, fxp, gem, msk, nfe, sk, stge etc) in CURRENT. -- Regards, Pyun YongHyeon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071212034114.GB68410>