From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19542 invoked by alias); 17 Oct 2011 02:48:09 -0000 Received: (qmail 19522 invoked by uid 22791); 17 Oct 2011 02:48:07 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD 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, 17 Oct 2011 02:47:53 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 65E5C2F78013 for ; Mon, 17 Oct 2011 03:47:52 +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 BGyytv3UFE3F; Mon, 17 Oct 2011 03:47:51 +0100 (BST) From: bugzilla-daemon@ecoscentric.com To: ecos-patches@ecos.sourceware.org Subject: [Issue 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: unassigned@bugs.ecos.sourceware.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Changed-Fields: Status In-Reply-To: References: X-Bugzilla-URL: http://bugzilla.ecoscentric.com/ Auto-Submitted: auto-generated Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Date: Mon, 17 Oct 2011 02:48:00 -0000 Message-Id: <20111017024750.F14942F78006@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: 2011-10/txt/msg00032.txt.bz2 Please do not reply to this email. Use the web interface provided at: https://bugzilla.ecoscentric.com/show_bug.cgi?id=1000910 Jonathan Larmour changed: What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |NEEDINFO --- Comment #12 from Jonathan Larmour 2011-10-17 03:47:45 BST --- I think this patch is looking very good now, including the further improvements you've made. Some (hopefully final) comments: > enc424j600_spi_init() does not loop infinitely anymore if there is no Ethernet > chip on SPI. Instead it tries finite number of times and CYG_FAILs (or returns > false) if there is no chip. The same mechanism is later implemented for > checking if the enc424j600's clock is ready. Number of retries and time between > retries are #defined in the source. Should they go to cdl? That's probably overkill. You can if you are feeling keen. >> - A trivial thing that would be nice: CYGNUM_DEVS_ETH_ENC424J600_FLOWC has >> values which could be friendlier when e.g. presented in the configuration tool. >> Any chance they could actually be just None and Auto? If you wanted, some >> #ifdef magic could still translate them if you preferred. > > There is a possibility for third option, besides no-flow-control and > on-chip-flow-control, that is in-driver-flow-control. I did not implement it, > but I think that it might be implemented in future. So, I decided to keep > CYGNUM_DEVS_ETH_ENC424J600_FLOWC unchanged. What I was suggesting wouldn't change that. It would just mean that in that case the legal_values now would be { "None" "On-chip" } and in future you could add "In-driver" to that. I'm just noticing that that option should probably be CYGSEM_... If you want to keep the rest of the code the same as it is now, you could add this in a header: #if (CYGSEM_DEVS_ETH_ENC424J600_FLOWC == On-chip) # define CYGSEM_DEVS_ETH_ENC424J600_FLOWC_ONCHIP_AUTO_FC #endif I see the stm3210e changes include a change to increase RAM use by 16k to allow for network-enabled redboot. That's quite a large change for something that most people don't need. I think a different MLT file would be better for that. You could either create a new HAL startup type, or you could just add a new option, and change CYGHWR_MEMORY_LAYOUT to be something like: calculated { (CYG_HAL_STARTUP == "RAM" ) ? (CYGMEM_HAL_CORTEXM_STM32_STM3210E_RESERVE_EXTRA_BASE_RAM ? "cortexm_stm3210e_eval_extrabaseram : "cortexm_stm3210e_eval_ram") : (CYG_HAL_STARTUP == "SRAM" ) ? "cortexm_stm3210e_eval_sram" : (CYG_HAL_STARTUP == "ROM" ) ? "cortexm_stm3210e_eval_rom" : (CYG_HAL_STARTUP == "ROMINT" ) ? "cortexm_stm3210e_eval_romint" : (CYG_HAL_STARTUP == "JTAG" ) ? "cortexm_stm3210e_eval_jtag" : "undefined" } Although I'm suggesting CYGMEM_HAL_CORTEXM_STM32_STM3210E_RESERVE_EXTRA_BASE_RAM for the option name, you may well be able to choose something with a better name. Let me know what you think. Jifl -- Configure issuemail: https://bugzilla.ecoscentric.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the issue.