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

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

[bugzilla-daemon]

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

           Summary: Enable the cache on Kinetis in RAM startup mode
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Cortex-M
            Status: NEW
          Severity: enhancement
          Priority: low
         Component: HAL
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: ilijak@siva.com.mk
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1783)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1783)
Kinetis cache fixes 120602

Currently Kinetis HAL enables the cache only in ROM startup mode, leaving RAM
mode to Redboot. After some time I find convenient to enable the cache in RAM
startup as well. Attached patch provides for this as well as some code
robustness fixes.

What should be right behavior in RAM startup? I would appreciate comments on
this issue.

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.

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

[bugzilla-daemon]

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

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jifl@ecoscentric.com

--- Comment #1 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-03 05:03:26 BST ---
You should use the existing standard defines in hal/common:
CYGSEM_HAL_ENABLE_DCACHE_ON_STARTUP and CYGSEM_HAL_ENABLE_ICACHE_ON_STARTUP.
Those options correctly default to 1 - we should always use caches if we can
unless the user configures otherwise.

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

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

[bugzilla-daemon]

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

--- Comment #2 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 21:09:58 BST ---
(In reply to comment #1)
> You should use the existing standard defines in hal/common:
> CYGSEM_HAL_ENABLE_DCACHE_ON_STARTUP and CYGSEM_HAL_ENABLE_ICACHE_ON_STARTUP.
> Those options correctly default to 1 - we should always use caches if we can
> unless the user configures otherwise.

Thank you for clarification Jifl. Then, that will be the case with this patch
so I am going to commit it accompanied with appropriate ChangeLog entry.

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.

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1783|0                           |1
        is obsolete|                            |

--- Comment #3 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-06 20:31:16 BST ---
Created an attachment (id=1795)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1795)
Kinetis cache fixes 120606-2129

Updated patch together with ChangeLog to be committed.

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

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

[bugzilla-daemon]

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

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|CURRENTRELEASE              |
         AssignedTo|unassigned@bugs.ecos.source |jifl@ecoscentric.com
                   |ware.org                    |

--- Comment #5 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-10 01:05:12 BST ---
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?

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?

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.

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

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.

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

 - 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. I intend to get to those patches later this
week.

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

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.

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

[bugzilla-daemon]

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

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |NEEDINFO

--- Comment #6 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-10 01:07:46 BST ---
And thanks for spending the time on this. Like you, I'm just trying to get
things to a good high standard. Don't confuse my nitpicking with any sort of
dissatisfaction :-). It's all good stuff really.

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.

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

[bugzilla-daemon]

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.

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

[bugzilla-daemon]

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

--- Comment #8 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-11 23:37:54 BST ---
Jifl

Here are considerations on your comment #6 that I have left out in comment #7.

(In reply to comment #5)

[snip]

> 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

I wander how have they landed here. They should appear just before (or
CYGINT_HAL_HAS_NONCACHEABLE might be parented by) CYGPKG_HAL_KINETIS_CACHE.

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

[snip]

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

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.

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

[bugzilla-daemon]

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

--- Comment #9 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-11 23:42:49 BST ---
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.

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

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

[bugzilla-daemon]

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

--- Comment #10 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-14 13:32:14 BST ---
Jifl

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.

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

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.

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

[bugzilla-daemon]

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.

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

[bugzilla-daemon]

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

--- Comment #12 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-18 22:16:14 BST ---
(In reply to comment #11)
> Hi Ilija,

I am extracting the discussion on FPU as topic in its own right, that might get
it's final shape in context of Cortex-M FPU support. The caches, on the other
hand have some options that need more detailed consideration.

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

I want to preserve also integral functionality of the part builder
CYGHWR_HAL_CORTEXM_KINETIS, including FPU option.
CYGHWR_HAL_CORTEXM_KINETIS_FPU selects FPU as Kinetis option and
CYGHWR_HAL_CORTEXM_FPU enables/disables FPU function so they aren't identical.
But it's true that they look alike so below is CYGHWR_HAL_CORTEXM_KINETIS_FPU
reshaped to resemble the other entries of the part builder
(CYGHWR_HAL_CORTEXM_KINETIS_FLEXNVM can be reshaped likewise).

        cdl_option CYGHWR_HAL_CORTEXM_KINETIS_FPU {
            display          "Floating Point Unit part name option"
            flavor           data
            no_define
            legal_values  { "D" "F" }
            default_value { "D" }
            requires      { CYGHWR_HAL_CORTEXM_FPU implies
                            CYGHWR_HAL_CORTEXM_KINETIS_FPU == "F" }
            description "Select/indicate whether the part has Floating Point
Unit. \"F\" - stands for 
            parts with FPU, while \"D\" for ones without."
        }

This form will distinguish CYGHWR_HAL_CORTEXM_KINETIS_FPU and
CYGHWR_HAL_CORTEXM_FPU in configtool and slightly simplify
CYGHWR_HAL_CORTEXM_KINETIS.

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.

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

[bugzilla-daemon]

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

--- Comment #13 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-23 06:58:21 BST ---
(In reply to comment #12)
> I want to preserve also integral functionality of the part builder
> CYGHWR_HAL_CORTEXM_KINETIS, including FPU option.
> CYGHWR_HAL_CORTEXM_KINETIS_FPU selects FPU as Kinetis option and
> CYGHWR_HAL_CORTEXM_FPU enables/disables FPU function so they aren't identical.

Okay, I see what you're getting at - so the part can match what the user has,
even if they're not actually using the FPU. That's fine.

> But it's true that they look alike so below is CYGHWR_HAL_CORTEXM_KINETIS_FPU
> reshaped to resemble the other entries of the part builder
> (CYGHWR_HAL_CORTEXM_KINETIS_FLEXNVM can be reshaped likewise).
> 
>         cdl_option CYGHWR_HAL_CORTEXM_KINETIS_FPU {
>             display          "Floating Point Unit part name option"
>             flavor           data
>             no_define
>             legal_values  { "D" "F" }
>             default_value { "D" }
>             requires      { CYGHWR_HAL_CORTEXM_FPU implies
>                             CYGHWR_HAL_CORTEXM_KINETIS_FPU == "F" }

You also need to have the converse:
             requires      { !CYGHWR_HAL_CORTEXM_FPU implies
                             CYGHWR_HAL_CORTEXM_KINETIS_FPU == "D" }

But other than that, that's fine by me.

>             description "Select/indicate whether the part has Floating Point
> Unit. \"F\" - stands for 
>             parts with FPU, while \"D\" for ones without."

Just say "Select".

Thanks!

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.

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

[bugzilla-daemon]

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.

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

[bugzilla-daemon]

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

--- Comment #15 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-26 21:53:57 BST ---
Hi Ilija,

(In reply to comment #14)
> (In reply to comment #11)
> > 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.

Admittedly I have only been looking at the K70 so maybe things are different
elsewhere, but SRAM isn't cacheable anyway (as per 3.5.4.1 Local memory
controller region assignment, page 126 of the K70 Ref manual) - presumably
because SRAM is as fast as cache. So we can ignore SRAM when it comes to
caching.

For the other regions, they are available mirrored, e.g. with one DRAM alias
writethrough at 0x08000000 - 0x0FFFFFFF, which is in the PC range and clearly
aimed at code, and DRAM higher up in the PS range which can also be writeback
mode, and is clearly aimed at data.

So I don't see the need to do things different with the cache, though it's
still reasonable to have the two layouts because of the difficulty in spreading
memory between regions - this could be solved with a little more linker
intelligence, so I've now submitted this for the future, not that it helps us
now: <http://sourceware.org/bugzilla/show_bug.cgi?id=14299>).

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

It doesn't have to really. Being read-only means you don't have to worry about
the data changing though, so I don't believe it makes any difference to my
suggestion on the memory layout and therefore caches. It's true the flash
driver would have to invalidate both caches, but then that's standard practice
for any flash driver.

We could theoretically relocate it as an option as SRAM is slightly faster than
flash (which has to go via the crossbar switch). But I'm not sure this is much
of a priority as I expect people would consider it much more important to save
the memory.

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

Anything which uses DMA is a concern.

[snip stuff which doesn't need reply]

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

Although it would be harmless to do so, there's already a .sram section
supported in the Cortex-M arch HAL's cortexm.ld, which would do the same thing,
so there isn't a need at the moment. Arguably it's better to just document one
standard one across all Cortex-M.

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

I think it's better doing it from the other end - the configuration tool should
have an option, essentially a filter, which could be used to show/hide
calculated packages/components/options, or inactive ones, or interfaces. I
don't think CDL should be getting involved in display semantics, just content.
Anyway this is a side-issue.

> (In reply to comment #8)
[snip, CYGPKG_HAL_KINETIS_CACHE]
> Probably it might be "calculated CYGINT_HAL_CACHE". I'll reconsider.

That would be fine too, the effect is the same since it's inactive if
!CYGINT_HAL_CACHE.


[more snip good stuff]

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

As above, I still don't think there's a need for them to be unified. But if I'm
wrong about that, then your first suggestion above looks correct.

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.

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

[bugzilla-daemon]

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

--- Comment #16 from Ilija Kocho <ilijak@siva.com.mk> 2012-09-04 21:36:26 BST ---
Created an attachment (id=1929)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1929)
RAM mirrored layout example

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

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

[bugzilla-daemon]

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

--- Comment #17 from Ilija Kocho <ilijak@siva.com.mk> 2012-09-04 22:19:19 BST ---
Hi Jifl

It's been a long time... Finally I got some time to revisit this bug.


(In reply to comment #15)

I did some experiments regarding Unified/Separate caching issue. Here are the
results and my observations. Other discussions are snipped off but not
rejected.

> Hi Ilija,
> 
> (In reply to comment #14)
> > (In reply to comment #11)
> > > 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.
> 
> Admittedly I have only been looking at the K70 so maybe things are different
> elsewhere, but SRAM isn't cacheable anyway (as per 3.5.4.1 Local memory
> controller region assignment, page 126 of the K70 Ref manual) - presumably
> because SRAM is as fast as cache. So we can ignore SRAM when it comes to
> caching.

I don't recall in what context I wrote the above?! Pse disregard.

> 
> For the other regions, they are available mirrored, e.g. with one DRAM alias
> writethrough at 0x08000000 - 0x0FFFFFFF, which is in the PC range and clearly
> aimed at code, and DRAM higher up in the PS range which can also be writeback
> mode, and is clearly aimed at data.

I tried to utilize this mirror memory in hope that caching code and data in
different caches will boost the Harvard. In Attachment #1929 the region
"program" placed at 0x08000000 is supposed to contain /code/ sections while
/data/ sections reside in "ram" region. The system crashes/resets when it comes
to initialization of .bss section. I was able to single-step but when I try to
run with full speed it doesn't work. My suspect is the collision between two
cache units over RAM, it looks like Harvard is trying to simultaneous
instruction fetch and data access.

> 
> So I don't see the need to do things different with the cache, though it's
> still reasonable to have the two layouts because of the difficulty in spreading
> memory between regions - this could be solved with a little more linker
> intelligence, so I've now submitted this for the future, not that it helps us
> now: <http://sourceware.org/bugzilla/show_bug.cgi?id=14299>).
> 
> > 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).
> 
> It doesn't have to really. Being read-only means you don't have to worry about
> the data changing though, so I don't believe it makes any difference to my
> suggestion on the memory layout and therefore caches. It's true the flash
> driver would have to invalidate both caches, but then that's standard practice
> for any flash driver.
> 

Considering above, I think that we could conclude that for both RAM and FLASH
it is hard (or not possible) to effectively enforce caching of instructions and
data in separate caches. If you agree I would keep unified caches and look
after your other remarks.

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.

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

[bugzilla-daemon]

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

--- Comment #19 from Ilija Kocho <ilijak@siva.com.mk> 2012-09-30 16:04:19 BST ---
(In reply to comment #18)

Hi again Jifl.

I am using the weekend to try separate cache configuration, and it seems
promising. I revisited your comments and now I realize that [from eCos point of
view] we can consider ICACHE as read-only and DCACHE as read/write?
The contents of attachment #1945 shall be part of a new patch in few days, (the
comment #18 is still applicable), so you don't need to spend time for review.

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.

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1795|0                           |1
        is obsolete|                            |
   Attachment #1929|0                           |1
        is obsolete|                            |
   Attachment #1945|0                           |1
        is obsolete|                            |

--- Comment #20 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-03 21:20:49 BST ---
Created an attachment (id=1948)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1948)
Kinetis variant - separate DCACHE-ICACHE 121003

Here come separate instruction and data caches. The main improvement is for
placing the code sections in PC segment for RAM startup. This unleashes Harvard
for caches that gives near-to-internal-SRAM performance for external DDRAM.

The attached patch (and the one that follows) also contain changes from
attachment #1945 described in comment #18

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.

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

[bugzilla-daemon]

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

--- Comment #21 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-03 21:26:50 BST ---
Created an attachment (id=1949)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1949)
Platform TWR-K70F120M Platform cache support support 121003

Main changes are additions of ramcod memory segment in MLT files for RAM
startup.

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.

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

[bugzilla-daemon]

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

--- Comment #22 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-07 18:23:57 BST ---
Created an attachment (id=1950)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1950)
Round-robin bus arbitration for copy-back DDRAM 121007

There is a subtle behaviour (with RAM start-up) when ICACHE is disabled, DCACHE
is in copy-back mode. Cache purge or sync gets preempted by code execution.
Since the macros for cache syncing and purging contain spin-check of cache
status, a deadlock happens, because of perpetual access to DDRAM, that has
higher priority than the cache flushing.

When ICACHE is enabled there isn't a deadlock, because [after initial misses],
the instructions are fetched from ICACHE so code fetches don't compete for
DDRAM.

The attached code avoids deadlock by introducing round-robin arbitration in
case start-up type is RAM and DCACHE uses copy-back scheme. The patch only
affects Kinetis variant and is incremental to attachment 1948.

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.

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1950|0                           |1
        is obsolete|                            |

--- Comment #23 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-08 20:11:02 BST ---
Created an attachment (id=1951)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1951)
Bus arbitration tweak for copy-back DDRAM 121008

This is an alternative to round-robin.
With a little tweak of bus arbitration priority, prior to spin-check of cache
status the priorities of PC and PS master of DDRAM slave are swapped (only in
case PC priority is bigger than PS priority) so code execution now can't stale
the DCACHE flushing (or any other operation). After the operation, the
priorities are restored, so practically all the time, the system works with
default settings (or whatever may have been set by application).

Note: This patch is incremental to attachment 1949.

Jifl,
I hope that now Kinetis CACHE has achieved desired functionality.

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.

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

[bugzilla-daemon]

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

--- Comment #24 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-08 20:15:23 BST ---
(In reply to comment #23)
> 
> 
> Note: This patch is incremental to attachment 1949 [details].
OOPs, I meant Attachment 1948.

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

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

[bugzilla-daemon]

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

--- Comment #25 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-13 10:27:40 BST ---
(In reply (addendum) to comment #23)

> Bus arbitration tweak for copy-back DDRAM 121008
> 

Another consideration: It is possible to swap PC and PS priorities permanently
for RAM startup. Then cache control macros will be shorter and there's no risk
of preemption.. The drawback, if any, is that crossbar arbitration priorities
for DDRAM will be different than the reset default. If implemented, this
inversion could be fixed or configurable with CDL.
Comments?


> This is an alternative to round-robin.
> With a little tweak of bus arbitration priority, prior to spin-check of cache
> status the priorities of PC and PS master of DDRAM slave are swapped (only in
> case PC priority is bigger than PS priority) so code execution now can't stale
> the DCACHE flushing (or any other operation). After the operation, the
> priorities are restored, so practically all the time, the system works with
> default settings (or whatever may have been set by application).
> 
> Note: This patch is incremental to attachment 1949 [details].
> 
> Jifl,
> I hope that now Kinetis CACHE has achieved desired functionality.
> 
> 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.

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

[bugzilla-daemon]

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

--- Comment #26 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-16 21:53:24 BST ---
Created an attachment (id=1961)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1961)
RAM startup sections revised incremental patch 121016

In RAM startup I have copied solution from Bug 1001623 which is suboptimal for
RAM startup. Here I submit a revision. The attached patch is incremental to
[combined] attachments 1948, 1949 and 1951.

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.

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1948|0                           |1
        is obsolete|                            |

--- Comment #27 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-26 11:57:45 BST ---
Created an attachment (id=1963)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1963)
Kinetis variant - separate DCACHE-ICACHE 121026

I reworked SRAM startups (mlt_kinetis_fl*_sram2s_sram.ldi) so they now resemble
RAM start-up. Prevouosly they were [trying to] emulate ROM start-up.

Since there are many changes accumulated, here I submit consolidated patches.

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.

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1949|0                           |1
        is obsolete|                            |
   Attachment #1951|0                           |1
        is obsolete|                            |
   Attachment #1961|0                           |1
        is obsolete|                            |

--- Comment #28 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-26 12:00:53 BST ---
Created an attachment (id=1964)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1964)
Platform TWR-K70F120M Platform cache support support 121026

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

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

[bugzilla-daemon]

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

--- Comment #29 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-26 12:03:23 BST ---
Created an attachment (id=1965)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1965)
Platform K60 K40 Platform/variant sync 121026

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

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

[bugzilla-daemon]

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

Bernd Edlinger <bernd.edlinger@hotmail.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bernd.edlinger@hotmail.de

--- Comment #30 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-10-27 08:56:30 BST ---
Hi Ilija,

just one or two things:

In macro CYGHWR_HAL_KINETIS_CACHE_WAIT
if the priorities do not need to be swapped, the delay loop seems to be
missing.

and generally the macro parameters should be used in brackets:
example:
#define CYGHWR_HAL_KINETIS_AXBS_CRS_PCTL(_x_)  ((_x_ <<
CYGHWR_HAL_KINETIS_AXBS_CRS_PCTL_S)...

here _x_ could be an expression and the macro should be written this way:
#define CYGHWR_HAL_KINETIS_AXBS_CRS_PCTL(_x_)  (((_x_) <<
CYGHWR_HAL_KINETIS_AXBS_CRS_PCTL_S)...

same for CYGHWR_HAL_KINETIS_AXBS_PRS_MASTER_S,
CYGHWR_HAL_KINETIS_AXBS_PRS_MASTER_PRIO, CYGHWR_HAL_KINETIS_AXBS_CRS_ARB,
CYGHWR_HAL_KINETIS_CACHE_WAIT: parameter lmem_p used wwithout brackets.

Regards,
Bernd Edlinger.

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

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1963|0                           |1
        is obsolete|                            |

--- Comment #31 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-27 12:48:01 BST ---
Created an attachment (id=1966)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1966)
Kinetis variant - separate DCACHE-ICACHE 121027

(In reply to comment #30)

Hi Bernd

> Hi Ilija,
> 
> just one or two things:
> 
> In macro CYGHWR_HAL_KINETIS_CACHE_WAIT
> if the priorities do not need to be swapped, the delay loop seems to be
> missing.

Thank you for the catch. Here I submit update.

> 
> and generally the macro parameters should be used in brackets:


Te parameters are mainly constants or other macros that (having parentheses of
their own) but, it's true there's a possibility for havoc. Protection is a good
practice indeed, so I updated macros macros too (used opportunity to fix some
older macros too).

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.

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

[bugzilla-daemon]

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

--- Comment #32 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-10-27 12:58:16 BST ---
(In reply to comment #31)
Hi Ilija,

how about this statement:

prs_tmp |= (m0 << CYGHWR_HAL_KINETIS_AXBS_PRS_MASTER_S(1)) || m1;

didn't you mean | instead of || ?

Regards,
Bernd.

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

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

[bugzilla-daemon]

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

--- Comment #33 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-27 13:14:53 BST ---
(In reply to comment #32)
> (In reply to comment #31)
> Hi Ilija,
> 
> how about this statement:
> 
> prs_tmp |= (m0 << CYGHWR_HAL_KINETIS_AXBS_PRS_MASTER_S(1)) || m1;
> 
> didn't you mean | instead of || ?

Sure :-(

For good or for bad, the bug "works" with default register values because m0 ==
0 and m1 == 1

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.

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

[bugzilla-daemon]

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

--- Comment #34 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-27 13:57:55 BST ---
Created an attachment (id=1967)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1967)
Kinetis variant - fix 1 [incremental to attachment 1966]

The attached patch fixes OR problem reported by Bernd in comment #32. The patch
is incremental to attachment #1966.

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.

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

[bugzilla-daemon]

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

--- Comment #35 from Jonathan Larmour <jifl@ecoscentric.com> 2012-11-03 06:42:59 GMT ---
Finally I reply....

First of all, to follow up a specific question you asked:

(In reply to comment #25)
> (In reply (addendum) to comment #23)
> 
> > Bus arbitration tweak for copy-back DDRAM 121008
> > 
> 
> Another consideration: It is possible to swap PC and PS priorities permanently
> for RAM startup. Then cache control macros will be shorter and there's no risk
> of preemption.. The drawback, if any, is that crossbar arbitration priorities
> for DDRAM will be different than the reset default. If implemented, this
> inversion could be fixed or configurable with CDL.
> Comments?

I don't have a very good feel for what the effects of this would be.
Specifically, can you imagine a situation when someone really would want the PC
priority to be higher than the PS one? I can't see it making a big difference
either way in the end, so I'm thinking a permanent change would be ok (although
if so, why wasn't it the reset default?).

As for the main patches.... I'm glad that the split cache scheme has made such
a great improvement in measured performance, as per our private email exchange.
It shows it was worth the effort!

One thing that strikes me as odd is the addition of CYG_HAL_STARTUP_DEF_ROM to
define CYG_HAL_STARTUP_ROM for RAM startup types. This seems potentially risky.
What problem was it intended to solve?

Other than that, I've reviewed everything already written in this issue, and
been through the current patches, and I don't see any reason not to commit the
patches. There might be a few essentially cosmetic things (spelling, typos,
etc.) that I could change, but minor touch-ups like that can be done after the
commit rather than going round the loop again.

Great work!

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.

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

[bugzilla-daemon]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1966|0                           |1
        is obsolete|                            |
   Attachment #1967|0                           |1
        is obsolete|                            |

--- Comment #36 from Ilija Kocho <ilijak@siva.com.mk> 2012-11-03 15:31:16 GMT ---
Created an attachment (id=1973)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1973)
Kinetis variant - separate DCACHE-ICACHE 121103

(In reply to comment #35)
> Finally I reply....

Many thanks...

> 
> First of all, to follow up a specific question you asked:
> 
> (In reply to comment #25)
> > (In reply (addendum) to comment #23)
> > 
> > > Bus arbitration tweak for copy-back DDRAM 121008
> > > 
> > 
> > Another consideration: It is possible to swap PC and PS priorities permanently
> > for RAM startup. Then cache control macros will be shorter and there's no risk
> > of preemption.. The drawback, if any, is that crossbar arbitration priorities
> > for DDRAM will be different than the reset default. If implemented, this
> > inversion could be fixed or configurable with CDL.
> > Comments?
> 
> I don't have a very good feel for what the effects of this would be.
> Specifically, can you imagine a situation when someone really would want the PC
> priority to be higher than the PS one? I can't see it making a big difference
> either way in the end, so I'm thinking a permanent change would be ok (although
> if so, why wasn't it the reset default?).

If PC priority is lower there shouldn't be danger of stale since soon after
instruction fetch is pre-empted by data access (from previous instruction), it
will cause the data transfer to seize (previous instruction ends) that will
unblock instruction fetch. Therefore, the set-up where PC has lower priority
than PS looks inherently more stable than the opposite.
However, I assume that it is most probable for users to expect reset-default
priorities upon start-up so I have left them unchanged. Also I can imagine
situation where user has set up PC higher priority then PS - then the
protective macro will be still necessary.

> 
> As for the main patches.... I'm glad that the split cache scheme has made such
> a great improvement in measured performance, as per our private email exchange.
> It shows it was worth the effort!

Indeed. I redid the tests and new results for old scheme are not as low (now I
have used different mirror, but there's still 30% boost.

> 
> One thing that strikes me as odd is the addition of CYG_HAL_STARTUP_DEF_ROM to
> define CYG_HAL_STARTUP_ROM for RAM startup types. This seems potentially risky.
> What problem was it intended to solve?

It shouldn't be there :(. I t is a ghost from my test to use technique from
FLASH start-up [Bug 1001623] - ROM start-up emulation. It was working but
copying initialized data from RAM to RAM makes little sense. Actually this
cdl_option was removed, but somehow re-appeared. Now I have removed (patch
attached) it again and retested everything.

> 
> Other than that, I've reviewed everything already written in this issue, and
> been through the current patches, and I don't see any reason not to commit the
> patches. There might be a few essentially cosmetic things (spelling, typos,
> etc.) that I could change, but minor touch-ups like that can be done after the
> commit rather than going round the loop again.
> 

So now I am going to commit, but I will leave this bug open for a while as a
reminder.

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.

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

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001606

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |RESOLVED
         Resolution|---                         |CURRENTRELEASE

--- Comment #37 from Ilija Kocho <ilijak@siva.com.mk> ---
No complains since it has been committed.
Closing.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

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

[bugzilla-daemon]

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

           Summary: Enable the cache on Kinetis in RAM startup mode
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Cortex-M
            Status: NEW
          Severity: enhancement
          Priority: low
         Component: HAL
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: ilijak@siva.com.mk
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1783)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1783)
Kinetis cache fixes 120602

Currently Kinetis HAL enables the cache only in ROM startup mode, leaving RAM
mode to Redboot. After some time I find convenient to enable the cache in RAM
startup as well. Attached patch provides for this as well as some code
robustness fixes.

What should be right behavior in RAM startup? I would appreciate comments on
this issue.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.