[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 #11 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-16 02:14:24 BST ---
Hi Ilija,

Sorry for the delay, I've been busy. I've snipped out some bits which don't
need a reply.

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

So as long as generic code does use HAL_DCACHE and HAL_ICACHE macros correctly,
I think they should still be able to stay distinct.

For example, consider a bootloader application which loaded the real
application, programmed it into off-chip flash (Flexbus) and jumped to it to
run it. It may have written to flash via the PS bus. There might be a (now
incorrect) cached value in the PC cache. But that's ok because the bootloader
application should still be calling HAL_ICACHE_INVALIDATE /
HAL_ICACHE_INVALIDATE_ALL. That's as true before as it would be on Kinetis.

(Our current flash drivers don't normally invalidate icache because it's
assumed we're not programming code which could modify the running program).

Now maybe there are more complicated situations where a Kinetis-specific
application or device driver does need to break this assumption. In that case,
because it's Kinetis-specific, it knows how the port is set up, so it can
choose the correct macros. For example calling both HAL_DCACHE_INVALIDATE and
HAL_ICACHE_INVALIDATE to ensure both caches are sorted out if that's really
what is needed.

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

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.

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

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

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

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

Sure, but where CYGHWR_HAL_CORTEXM_KINETIS currently has:
        calculated { "MK" . CYGHWR_HAL_CORTEXM_KINETIS_SUBFAM .
            (CYGHWR_HAL_CORTEXM_KINETIS_FPU ? "F" : "D") .
            (CYGHWR_HAL_CORTEXM_KINETIS_FLEXNVM ? "X" : "N") .
            CYGHWR_HAL_CORTEXM_KINETIS_FLASH_NAME }
you could just change that to:
        calculated { "MK" . CYGHWR_HAL_CORTEXM_KINETIS_SUBFAM .
            (CYGHWR_HAL_CORTEXM_FPU ? "F" : "D") .
            (CYGHWR_HAL_CORTEXM_KINETIS_FLEXNVM ? "X" : "N") .
            CYGHWR_HAL_CORTEXM_KINETIS_FLASH_NAME }

and preserve that behaviour? It seems better than have two options which
effectively do exactly the same thing, and have to have the same setting as
each other.


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

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

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

(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!

;-)

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

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

Cheers,

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.