[Bug 1001561] Internal flash driver for Freescale TWR-K60N512 board

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

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 <jifl@ecoscentric.com> 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.