From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4260 invoked by alias); 2 Apr 2012 20:21:51 -0000 Received: (qmail 4067 invoked by uid 22791); 2 Apr 2012 20:21:47 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED X-Spam-Check-By: sourceware.org Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 02 Apr 2012 20:21:32 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 4D21D2F78003 for ; Mon, 2 Apr 2012 21:21:31 +0100 (BST) Received: from mail.ecoscentric.com ([127.0.0.1]) by localhost (hagrid.ecoscentric.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JP7plLJqrwIE; Mon, 2 Apr 2012 21:21:29 +0100 (BST) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1000910] New port: Ethernet over SPI driver for ENC424J600 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: eCos X-Bugzilla-Component: Patches and contributions X-Bugzilla-Keywords: X-Bugzilla-Severity: enhancement X-Bugzilla-Who: jifl@ecoscentric.com X-Bugzilla-Status: NEEDINFO X-Bugzilla-Priority: low X-Bugzilla-Assigned-To: jifl@ecoscentric.com X-Bugzilla-Target-Milestone: --- X-Bugzilla-Changed-Fields: In-Reply-To: References: X-Bugzilla-URL: http://bugs.ecos.sourceware.org/ Auto-Submitted: auto-generated Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Date: Mon, 02 Apr 2012 20:21:00 -0000 Message-Id: <20120402202129.4CE5C2F78010@mail.ecoscentric.com> Mailing-List: contact ecos-patches-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: ecos-patches-owner@ecos.sourceware.org X-SW-Source: 2012-04/txt/msg00018.txt.bz2 Please do not reply to this email. Use the web interface provided at: http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000910 --- Comment #16 from Jonathan Larmour 2012-04-02 21:21:22 BST --- Hi Ilija, Let's try and get this committed. I've gone through the current version of the patch right from the beginning again. Driver comments: - In the CDL you don't want to use !is_active(CYGINT_IO_ETH_INT_SUPPORT_REQUIRED), is it will always be active if the package is included which it always would be. Instead you want something like: requires { CYGINT_IO_ETH_INT_SUPPORT_REQUIRED implies (CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_VECTOR != "\"\"") } (and the same for _PRIORITY) - I think I'd prefer the option CYGNUM_DEVS_ETH_ENC424J600_ACCEPTABLE_PACKET_SIZE to be CYGNUM_DEVS_ETH_ENC424J600_MAX_RX_SIZE since that's what it is really. - In CYGNUM_DEVS_ETH_ENC424J600_TXBUF_SIZE, I think the lowest transmit buffer size could be lower than 1518. lwIP for example allows you to limit the packet size, so it would be possible for a user to guarantee that larger packets weren't sent by the stack. But I'd leave the default as it is. - Purely for consistency, please change CYGNUM_ETH_ENC424J600_USER_INTERRUPT_VECTOR -> CYGNUM_DEVS_ETH_ENC424J600_USER_INTERRUPT_VECTOR - "treshold" should be "threshold" - For CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_PRIORITY and CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_VECTOR, you don't need to have a separate "user" option. You just need to have the default_value of CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_PRIORITY to be: is_active(CYGNUM_HAL_SPIETH_INTERRUPT_PRIORITY) ? CYGNUM_HAL_SPIETH_INTERRUPT_PRIORITY : "\"\"" That way it also allows users to override the HAL if they've hooked it up differently. - On committing I will probably just tidy up a few of the CDL descriptions for English clarity, please don't be offended! STM3210e changes comments: - It may need to be updated due to recent commits in the stm3210e HAL. - You can remove these from CYGNUM_ETH_SPIETH_HAL_INTERRUPT_VECTOR: requires { CYGHWR_HAL_SPIETH_INTERRUPT_PIN != "" } requires { CYGHWR_HAL_CORTEXM_STM32_SPIETH_INTERRUPT_PORT != "" } - Think of how the CDL will look in the graphical config tool - "STM32 HAL resources assigned to SPI Ethernet controller" is a very long string to display. It would be better just as "Optional ENC424J600 SPI Ethernet" - In CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS, rather than setting the legal values from the enabled buses, it should be the other way round - require the buses to be enabled based on the setting. In other words: legal_values { 1 2 3 } requires { (CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS == 1) implies CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS1 } requires { (CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS == 2) implies CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS2 } requires { (CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS == 3) implies CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS3 } - CYGHWR_HAL_SPIETH_INTERRUPT_PIN can have just "legal_values 0 to 15" - I don't think you should be putting anything in stm3210e_eval_misc here. That should be kept for HAL stuff, and this is a very optional peripheral add-on. It should be a separate file. But even then, what you do doesn't seem quite right. Any port specific initialisation should be performed via the enc424j600_spi_init() function. The eth driver should use a HAL provided hook. It can get this by including but in practice it will usually be defined in the platform's plf_io.h. For example for stm3210e's plf_io.h: struct cyg_netdevtab_entry; __externC cyg_devs_cortexm_stm3210e_enc424j600_init( struct cyg_netdevtab_entry * ); #define CYG_DEVS_ETH_ENC424J600_PLF_INIT( _tab_ ) \ cyg_devs_cortexm_stm3210e_enc424j600_init( _tab_ ) where "_tab_" is the cyg_netdevtab_entry passed to enc424j600_spi_init(). This would also get rid of the very yucky duplication of netdev_lookup(). Comments for both: The ChangeLog format still isn't quite right I'm afraid. It doesn't mention the files affected. It's probably not worth worrying about this for the eth driver since it's a new package (but please do it for new changes in that package), but you should follow the format for the existing ChangeLogs. Sorry that there's rather more comments than I'd have expected at this point, but I've been through it in more detail this time than before. Jifl -- Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.