[Bug 1001606] Enable the cache on Kinetis in RAM startup mode

[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=1001606

--- Comment #7 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-10 21:35:05 BST ---
Hi Jifl

(In reply to comment #5)
> Hi Ilija,
> 
> Looking at this patch, I decided to look a bit closer at the Kinetis. Some
> things to do with caches strike me as a bit unusual, but hopefully you can
> clarify where my misunderstandings lie:
> 
> The cache is split into processor code (PC) and processor system (PS) buses.
> Code accesses go through the PC bus, and data through the PS bus. Yet
> hal_cache.h treats both as dcache, which doesn't seem right - PC accesses are
> equivalent to the icache surely? Shouldn't we be splitting things up
> appropriately between the HAL_DCACHE and HAL_ICACHE macros?

Cortex-M is _modified_ Harvard architecture. Both PC and PS can both fetch
instructions and transfer data. Only, if instruction fetch is on PC and data on
PS then they can be simultaneous (Harvard).
FWIW both Kinetis caches are unified. Cached resources (FLASH, DDRAM, etc) have
multiple ports that are mirrored at different address segments and attached to
either PC or PS. Dependent on mapped address the same physical location may be
cached in either cache (I can imagine a havoc caused by same locations cached
in both).

This implies that flushing, invalidation and other cache management must be
performed on both caches. On the other hand it may be a good idea to provide
independent enabling/disabling of individual cache modules by means of
DCACHE/ICACHE - with appropriate notes in CDL and SGML documentation.
Please comment.

> 
> Given the division of SRAM memory into PC and PS halves, how are .2ram sections
> (such as used by many flash drivers for off-chip flash) supported? They are not
> mentioned in the memory layouts. FlexBus does seem to allow for off-chip flash
> so these flash drivers could be used on production hardware. Should your
> "code_sram" sections be instead changed into something that would work with the
> existing .2ram.* section naming for consistency with the rest of eCos?

Kinetis have 2 SRAM modules SRAM_L and SRAM_H accessible through PC and PS
respectively. In some applications it may be convenient to put some code in
SRAM_L (such as for deterministic response). It could be either copied from
FLASH or loaded (from mass storage or net). Please note that mlt files under
kinetis/var directory are dedicated to single chip configurations (no external
flash). Of course it's not the case with platform mlt files (please refer to
Kinetis SGML doc).

Regarding .2ram section naming, frankly, I wasn't aware of it so far. I'll
study the eCos ref and check if it's applicable. Can you point me some
examples?

> 
> A minor thing, but does CYGPKG_HAL_KINETIS_CACHE really want to be an option?
> It makes things inconsistent with CYGSEM_HAL_ENABLE_(DCACHE|ICACHE)_ON_STARTUP.
> I think it is generally a safe assumption that if cache is present, users will
> want to use it - the only question might be exactly at what point it becomes
> enabled (and of course details about non-cacheable regions, writethru versus
> writeback etc.), although developers may temporarily want to disable it if
> debugging a problem which may be cache-related, but it would end up being
> enabled again for production.

Cache is optional module on Kinetis, found (at present) only on parts with
operating frequencies of 120 and 150 MHz so it is left to platform to implement
it.
Note: Not  to be confused with caching function provided as part of FLASH
controller.

> 
> Not related to the particular patch, but still to do with caching
> configuration... Some subsystems have their caching configuration set under
> CYGPKG_HAL_KINETIS_CACHE, whereas others have them set in their own tree - or
> at least SDRAM/DDR does, but I haven't looked more widely for others. And the
> two key interfaces CYGINT_HAL_CACHE and CYGINT_HAL_HAS_NONCACHEABLE live under
> a separate component CYGHWR_HAL_KINETIS_MEMORY_RESOURCES. I think related
> options ought to be kept in the same place. OR if there's a good reason why
> not, a note should be placed in the description e.g. of
> CYGPKG_HAL_KINETIS_CACHE pointing users in the right direction of other
> cache-related configuration options. In line with my thoughts at the top, I
> think the two interfaces could be brought under CYGPKG_HAL_KINETIS_CACHE which
> would then become "flavor none/no_define", with the active_if then applying to
> CYGHWR_HAL_NON_CACHABLE. hal_cortexm_kinetis_conf_cache_regions() can be called
> from HAL_DCACHE_ENABLE() and HAL_ICACHE_ENABLE() appropriately, so it's still
> only called if actually needed (and linker garbage collection will remove it if
> never called).

This needs some attention. I'll re-consider and come back.

> 
> There are some other issues separate to the cache I have noticed too, and I may
> as well mention them here (but I can submit in a new bug if you prefer). Such
> as:
> 
>  - CYGHWR_HAL_KINETIS_FLEXNVM_FLASH_SIZE has the same CDL display string as
> CYGHWR_HAL_KINETIS_FLASH_SIZE.

Bl**** copycat, I'll fix.


> 
>  - The descriptions of options like CYGHWR_HAL_ENET_TCD_SECTION appear to
> conflict with their default settings.
> 
>  - CYGINT_HAL_CORTEXM_KINETIS_RTC has no display string.

I guess I left it empty because interfaces are invisible. I shall add one.

> 
>  - It appears that CYGHWR_HAL_CORTEXM_KINETIS_FPU is effectively redundant
> (since it's keyed off CYGHWR_HAL_CORTEXM_FPU), but I suspect this has now been
> dealt with in your recent patches.

It's actually Kinetis' specific. FPU is optional on Kinetis and it is reflected
in part names. If you toggle CYGHWR_HAL_CORTEXM_KINETIS_FPU in configtool you'l
notice changes in the part's name ('F' <-> 'D'). Optionally this option could
be a data CDL with legal values "D" and "F" (more consistent with other part
name segments) but bool seems more natural to me. Ref: The /part name building/
is described in SGML doc. 


> I intend to get to those patches later this
> week.

Looking forward :). Please regard them as a draft.

> 
> - Incidentally some of the indentation in the kinetis CDL is a bit off, 
> e.g. underneath CYGHWR_HAL_KINETIS_FLEXNVM_FLASH_SIZE, at the end of
> CYGPKG_HAL_CORTEXM_KINETIS_FLEXBUS and beginning of
> CYGHWR_HAL_KINETIS_FLASH_CONF.

I'll check.

> 
> I realise it would have been better if issues like these had been raised
> earlier, especially during submission of the initial port, and that's my own
> fault for not really looking at things much while I was only working part-time
> for eCosCentric. I'm trying to improve the balance now.

Thank you for your time.
For my forthcoming (and/or pending) patches you (as any maintainer) can request
some finite/reasonable time for review by just announcing his (pity no ladies
:) ) intention to the bug.

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.