This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001561

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ilijak@siva.com.mk

--- Comment #6 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-22 17:34:01 BST ---
(In reply to comment #5)
> Hi Ilija
> 
> I sent the e-mail requested for the copyright assignment and I am waiting for
> their answer.

Thanks, then, let's begin and prepare your code for commit while paperware is
traveling.

> I updated the attachments :
> 
> - I move the common code to the variant level.
> 
> - I separated them as you recommended.
> 
> - I deleted the temporary files (sorry about that).
> 
> Also I figured out why I wasn't able to access some part of the flash. In fact
> speculation logic and cache aliasing (which are enable by default) are not
> supported on some tower boards. See there for more informations :
> http://forums.freescale.com/t5/Kinetis-ARM-Cortex-M4/Accessing-the-FLASH-causing-BUS-fault/m-p/78873#M509

Thank you for the pointer.

> 
> I added an option to disable these options in the cdl file. The driver should
> now work properly.

Good usage of eCos configureability.

I haven't yet tried the driver on target, here are some notes based on code
review.

First some formal notes:

I should have pointed this earlier (sory), here you will find some guidance how
to prepare eCos code:
http://ecos.sourceware.org/patches-criteria.html

That being said here are few notes:

Update ChangeLog in existing packages (i.e. Kinetis HAL) and add one to new
ones (the driver).

Convert tabs to spaces (tab size 4 is OK).

Make sure that Copyright banners are accurate. This includes year(s) in
Copyright line.

Line length should be typically less than 78 characters. This is a soft rule
but please try to reformat your code accordingly.

In file devs/flash/freescale/kinetis/current/src/kinetis_flash.c:

function kinetis_flash_init(): Your code seems correct, but FYI eCos provides a
set of hardware abstraction macros such as HAL_READ_UINT32() HAL_WRITE_UINT32()
for hardware abstraction. Device registers are typically accessed either
structured (as you do for cyghwr_hal_kinetis_flash_t) or by means of HAL macro.
Would you mind editing this part?

Example:

    cyg_uint32 regval;

    HAL_READ_UINT32(CYGHWR_HAL_KINETIS_FMC_PFB0CR, regval);
    regval &= ~CYGHWR_HAL_KINETIS_FMC_PFBCR_BIPE;
    HAL_WRITE_UINT32(CYGHWR_HAL_KINETIS_FMC_PFB0CR, regval);

Lines like following:
    CYGHWR_HAL_KINETIS_SIM_P->scgc6 |= 0x01UL;

There are macros for SCGS bit fields in var_io.h that you could use (If some
macro is missing feel free to add)

Note: FTFL clock is already being enabled during system init (all clock are)
but do not remove this line from FTFL driver, in some future upgrade all
drivers should have such and I intend to remove general clock enabling.

I guess that flashCommandSequence() could be static. eCos is being linked to
application(s) so let's keep exported names to minimum.

Could query[] be more general: "Freescale Kinetis internal flash" or "Kinetis
FTFL" ?

In file devs/flash/freescale/kinetis/current/src/kinetis_flash.h:

KINETIS_FLASH_BLOCK_SIZE: There are some devices with block size of 4KiB so it
should be configurable or inferred from device type. I have been informed that
there is some ambiguity in K60 devices as there are devices with both 2 KiB and
4 KiB block size, so to be on a safe side I would provide a CDL:

    cdl_option CYGNUM_DEVS_KINETIS_FLASH_BLOCK_SIZE {
        display "Block size"
        legal_values { 0 0x800 0x1000 }
        default_value 0
        description ".... 0 - Auto ...."
   }

In addition the platform HAL can override. Also check if some other value can
be configurable/inferred or read from Kinetis registers.

Regarding configuration options it would be good to cross-reference other
device manuals, typically K40 and K70 (that we have or will have soon in eCos).

Ilija

-- 
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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]