From owner-freebsd-drivers@FreeBSD.ORG Wed Dec 12 03:45:10 2007 Return-Path: Delivered-To: freebsd-drivers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 253CE16A47C for ; Wed, 12 Dec 2007 03:45:10 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: from wa-out-1112.google.com (wa-out-1112.google.com [209.85.146.178]) by mx1.freebsd.org (Postfix) with ESMTP id E73C413C45D for ; Wed, 12 Dec 2007 03:45:09 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: by wa-out-1112.google.com with SMTP id k17so129157waf.3 for ; Tue, 11 Dec 2007 19:45:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:received:received:date:from:to:cc:subject:message-id:reply-to:references:mime-version:content-type:content-disposition:in-reply-to:user-agent; bh=NXWJgODyFaSxZo4esmoZIltJ+juOJROHYxi8kI+enxs=; b=g5j4v20TcsTQfR+X6RkxCcrulTQEte8FDROfxDMYc5vKp9fT5Pual9s1kZiqpbPGG+KukjXcvD9tclYepBTHpAs86yMMmwGtQbTyx5uIYcwUPnNv2aPmM76+OfseucPGqrfJq00wv+IAN9+9kdRgysDfJOh8YP4Ul/Jb19P3TtA= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:reply-to:references:mime-version:content-type:content-disposition:in-reply-to:user-agent; b=B1WvsXqk15v4ua0AI8qQhl5ySaZY1pW5UfDKF+yplUu693z283mdmJaNTDyvuwDpdMAP2zRLTbaT5Wcr7qvSe/9cls3j8g7yDu6miPPD54R5cQPIKsLujqWDsbOX98F0qByhI6IyJ6xMiGsLJVL4kKNtsGzntW3+sgOUbxrxToc= Received: by 10.115.33.1 with SMTP id l1mr243307waj.17.1197431108555; Tue, 11 Dec 2007 19:45:08 -0800 (PST) Received: from michelle.cdnetworks.co.kr ( [211.53.35.84]) by mx.google.com with ESMTPS id j6sm1705273wah.2007.12.11.19.45.05 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 11 Dec 2007 19:45:07 -0800 (PST) Received: from michelle.cdnetworks.co.kr (localhost.cdnetworks.co.kr [127.0.0.1]) by michelle.cdnetworks.co.kr (8.13.5/8.13.5) with ESMTP id lBC3fGTk069076 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 12 Dec 2007 12:41:16 +0900 (KST) (envelope-from pyunyh@gmail.com) Received: (from yongari@localhost) by michelle.cdnetworks.co.kr (8.13.5/8.13.5/Submit) id lBC3fElX069075; Wed, 12 Dec 2007 12:41:14 +0900 (KST) (envelope-from pyunyh@gmail.com) Date: Wed, 12 Dec 2007 12:41:14 +0900 From: Pyun YongHyeon To: Alexander Pohoyda Message-ID: <20071212034114.GB68410@cdnetworks.co.kr> References: <200712102046.27504.alexander.pohoyda@gmx.net> <20071211012711.GB64299@cdnetworks.co.kr> <20071211074509.281710@gmx.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20071211074509.281710@gmx.net> User-Agent: Mutt/1.4.2.1i Cc: freebsd-drivers@freebsd.org Subject: Re: SiS 190 NIC driver X-BeenThere: freebsd-drivers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: pyunyh@gmail.com List-Id: Writing device drivers for FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Dec 2007 03:45:10 -0000 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 mentions: SiS191 Gigabit LAN & SiS190 LAN Driver(v2.03) > > Other websites have it wrong, I guess, e.g.: 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 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