From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12649 invoked by alias); 11 Jul 2012 05:14:11 -0000 Received: (qmail 12640 invoked by uid 22791); 11 Jul 2012 05:14:10 -0000 X-SWARE-Spam-Status: No, hits=-2.8 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; Wed, 11 Jul 2012 05:13:56 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 0ECBF2F780CA for ; Wed, 11 Jul 2012 06:13:55 +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 X3H5+wvZZ3t5; Wed, 11 Jul 2012 06:13:53 +0100 (BST) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1001561] Internal flash driver for Freescale TWR-K60N512 board 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: UNCONFIRMED X-Bugzilla-Priority: low X-Bugzilla-Assigned-To: ilijak@siva.com.mk 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: Wed, 11 Jul 2012 05:14:00 -0000 Message-Id: <20120711051353.20E332F78005@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-07/txt/msg00009.txt.bz2 Please do not reply to this email. Use the web interface provided at: http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001561 --- Comment #33 from Jonathan Larmour 2012-07-11 06:13:40 BST --- Hi Nicolas, As well as Ilija's comments, I also have a few comments too before this is committed: - The Flash package needs a ChangeLog file - The whitespace could be tidied a little in the CDL file. Just line things up and so on. - There's an unnecessary couple of blank lines added in var_io.h just at the end of the FMC definitions. - Do not include pkgconf/redboot.h. RedBoot is not the only thing that can use the flash driver. - I don't think the flash driver should be instantiated in kinetis_misc.c. It should either go in kinetis_flash.c along with everything else, or go in individual platform HALs, to allow them the ability to override or disable (like the STM32 does with its internal flash driver). - The cyg_kinetis_flash_dev structure can be completely empty. Its contents are not needed unless you are dynamically working out the sizes like the STM32 does (see below). - There's no need to re-declare hal_kinetis_flash_conf_p in kinetis_flash.c. Just include var_io.h. - kinetis_flashEraseAllBlocks and kinetis_flashEraseBlock are not used. They should be either removed or put in #if 0 to silence warnings. Although if you do want to keep flashEraseBlock, then I suggest checking the requested address is block aligned first. - I know you copied kinetis_flash_query() from somewhere else (STM32), but nevertheless the query string can be 'const' (which the STM32 driver should have as well). And add the 'e' onto 'Freescal'. Or better still, just use something shorter like "Internal" or "On-chip", to save a little flash. Also it would seem better to change this: memcpy( data, query, sizeof(query)); to: if ( sizeof(query) < len ) len = sizeof(query); memcpy( data, query, len ); - In flashCommandSequence(), please either add a comment on the empty while loops, or use CYG_EMPTY_STATEMENT; instead. - In response to Ilija's comments, for his point (2) I don't think there's anything that can sensibly be done. All flash drivers run the risk of messing things up so you can't boot the board. And for his point (3), I agree it would be better to make it more like the STM32 driver, and work things out from the part. Although I disagree with Ilija and wouldn't say to check it against the hardware (unless CYGPKG_INFRA_DEBUG is defined maybe). Thanks for working through this and contributing Nicolas! 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.