[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 #14 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-23 23:26:33 BST ---
Hi Jifl

(In reply to comment #11)
> Hi Ilija,
> 
> Sorry for the delay, I've been busy. I've snipped out some bits which don't
> need a reply.

Thank you for your time.

> 
> (In reply to comment #7)
> > 
> > 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).
> 
> Good grief. I hadn't realised it was quite like that. Nevertheless......
> 
> > This implies that flushing, invalidation and other cache management must be
> > performed on both caches.
> 
> Not necessarily. I've had a closer look at the Kinetis docs now and from what I
> can see, the intention of the Kinetis designers is that you run code via the PC
> bus, and access data through the PS bus. And indeed everything you would need
> to access in order to run code is available from a mirrored address mapping
> associated with the PC bus; likewise SRAM and SDRAM can be accessed via the PS
> bus.
> 
> So what we should be doing is saying that the standard behaviour for Kinetis
> ports is for all code to run at addresses from 0x0 - 0x1fffffff in order to go
> via the PC bus; and all data is accessed from 0x20000000 - 0xffffffff to go via
> the PS bus. Then we can say that HAL_ICACHE macros correspond to the code
> addresses, and HAL_DCACHE macros correspond to the data.
> 

Then I would consider the following:

1. Separate cacheing is only applicable to mlt_*sram2s* memory layouts. I'd
expect the majority of users to be happier with mlt_*unisram*, especially on
devices with smaller SRAM.

2. The internal FLASH is only accessible at base address 0x0 i.e. from PC bus.
This implies that .rodata has to be relocated in either SRAM_U or external RAM
(for systems that have external RAM).
Probably for external RAM it is not a big problem (although we probably need to
do some profiling).
For systems with only internal SRAM we it's a trade-off between limited SRAM
resource and benefits of separated caches (as we still need caches for the
FLASH). AFAICT, with the application of .noncache for bus master shared memory,
we won't have much need for cache invalidations, and even if we have
invalidations they are related to loader and/or debugger, therefore
invalidation of both caches won't be a big penalty.

[snip]

> 
> Uncached mappings should be able to be accessed using the standard approach of
> CYGARC_UNCACHED_ADDRESS macro in var_io.h/plf_io.h (as per the HAL doc on
> "Address Translation").

I'll define this macro and it's siblings.

> 
> If instead we keep the current unified cache model, then most of the time, for
> example, we will be invalidating one of the caches unnecessarily.

Here I refer to my consideration on SRAM trade-off.

> 
> > 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.
> 
> Can you think of a scenario where treating DCACHE/ICACHE separately would cause
> a problem from *generic* code such as an non-Kinetis specific application or
> generic device driver? There may be one, I just haven't thought of it.
> 
> So as above I think being able to do it independently would be good.
> 
> > 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?
> 
> The main users are flash drivers (although there's at least one ethernet
> driver. For example look for .2ram in
> devs/flash/amd/am29xxxxxv2/current/src/am29xxxxx_aux.c. Although it's not
> documented or stated, the intention is for it to be used specifically for
> placing code explicitly in RAM rather than ROM/flash. This could also be used
> sometimes for selective performance tuning if RAM is faster than ROM, but there
> isn't room for the whole application in RAM.
> 
> cortexm.ld already just sticks .2ram input sections in the .data output
> section, but we could of course provide some form of override to allow better
> placement on Kinetis.

May we introduce another input section, something like .2sram? I would keep
.2ram as is, it would be useful for large program sections that can't fit in
SRAM. I can imagine applications that may make use of both.

> 
> > > 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.
> 
> Sure, but putting my question another way, should we not just have
> CYGPKG_HAL_KINETIS_CACHE be "calculated 1" (and still active_if
> CYGINT_HAL_CACHE)? That's what causes the inconsistency I'm referring to.

This seems redundant with the "reply to comment #8" below. So I am replying
there.

> 
> > >  - 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.
> 
> Only invisible in the graphical config tool, and that's a decision I don't
> entirely agree with (or at least, I think it should be possible for the config
> tool to be able to optionally display them, so hopefully in the future it
> will).

What about adding a command "(in)visible" to cdl_option and cdl_component?

> 

[ FPU discussion snipped out ref comment #12 ]

> 
> 
> (In reply to comment #8)
> > > 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).
> > 
> > With the change above I would like to keep CYGPKG_HAL_KINETIS_CACHE as is. As
> > I noted in comment #7, the cache is optional on Kinetis - therefore it's
> > absence is more intuitive presented if it's grayed for cache-less parts.
> 
> Sorry, I wasn't clear.... what I was trying to say was that it doesn't need to
> be an option which can be set by the user. Instead it should just be calculated
> 1 if the hardware says there's a cache.

Probably it might be "calculated CYGINT_HAL_CACHE". I'll reconsider.

> 
> > >  - The descriptions of options like CYGHWR_HAL_ENET_TCD_SECTION appear to
> > > conflict with their default settings.
> > 
> > I guess you refer to sram being brought in conjunction with the cache. But
> > SRAM is non-cachable (always) and that's stated in K70 manual (Table 3-41.
> > Cache regions). I guess it wouldn't be strange to K70 users.
> 
> I still think the description could be improved to clarify this given the
> naming... if SRAM is already non-cacheable, what does .noncache do then? (*I*
> now know what it does having looked into it, but I'm just thinking about what
> the users will see). Yes there's an mention in the documentation about
> .noncache, but that's not clearly related to these options and anyway the CDL
> should also describe a bit more about options (I do have a plan at some point
> to automagically generate a configuration reference in docbook format from the
> package CDL, which would be put at the end of each package documentation as an
> appendix).
> 
> Anyway, how about this appended to each of the options under
> CYGHWR_HAL_NON_CACHABLE: "Selecting \".sram\" will result in the memory being
> placed in on-chip SRAM. Selecting \".noncache\" will result in the memory being
> placed in a non-cacheable mapping of off-chip SDRAM."
> 

Yes. I first noticed it at Heathrow and in London's Underground ;-) English
like full-sentence explanations that leave little doubt. I'll comply.


> Incidentally you might also want to fix the typo "descriptos" to "descriptors"
> :-).
Tnx

> 
> (In reply to comment #9)
> > Jifl
> > 
> > Here are considerations on your comment #6 that I have left out in comment #7.
> > 
> > Correction (sorry), actually I refer to your comment #5.
> > 
> > (In reply to comment #5)
> > 
> > Comes comment 8.
> 
> I'm glad that here in comment #11 I can confirm that I understand comment #9,
> and its correction of comment #8 to refer to comment #5 instead of comment #6,
> other than comments in comment #7.
> 
> So that's all cleared up then!

sure!

> 
> ;-)
> 
> (In reply to comment #10)
> > I am applying changes and I have some doubts.
> > 
> > Considering that Kinetis have 2 unified caches:
> >     - Is it OK to independently disable/enable them by DCACHE_ENABLE/
> > DCACHE_DISABLE respectively (with appropriate documentation in CDL descriptions
> > and SGML)? The present state treats them as unified cache.
> 
> As above, I _think_ it may be better not to treat them as unified, but if you
> can think of a counter-example where this causes a problem please do - I'm sure
> you've thought about this more than I have.
> 
> Again the alternative is unnecessary cache operations.

I guess that by now you have read my consideration above.

> 
> >     - What would be appropriate setting for DCACHE_SIZE / ICACHE_SIZE and
> > DCACHE_WAYS / ICACHE_WAYS (Each Kinetis cache size is 8KiB with 2 ways)?
> > 
> > Current setting is:
> > #define    DCACHE_SIZE (2*8192)
> > #define    DCACHE_WAYS (2*2)
> > 
> > #define    ICACHE_SIZE (2*8192)
> > #define    ICACHE_WAYS (2*2)
> 
> Based on my suggestion above, we would treat each cache independently then, so:
> #define    DCACHE_SIZE (8192)
> #define    DCACHE_WAYS (2)
> 
> #define    ICACHE_SIZE (8192)
> #define    ICACHE_WAYS (2)

What would be your suggestion if we treat(ed) them as unified?

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