This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


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

RE: MPC8260 cache patch


On Thu, 2003-03-27 at 11:53, Patrick Doyle wrote: 
> For those of you who are still interested (is anybody still interested?) I
> tried removing the invalidate lines for 'HAL_FLASH_CACHES_OFF()'.  As
> expected, this made the problem go away, so I have (hopefully) attached a
> patch to that effect.
> 
> Here are my thoughts:
> 'HAL_FLASH_CACHES_OFF()' syncs the data cache and then disables it.  This
> makes the cache be coherent with main memory, but does not necessarily make
> main memory be coherent with the cache.  Code that calls
> 'HAL_FLASH_CACHES_OFF()' should always be limited to code that is going to
> manipulate the flash _ONLY_ and not main memory.  (There is a potential
> problem here -- the stack is in main memory, but as long as the stack is not
> in the portion of main memory that is referenced by the call to
> 'HAL_DCACHE_SYNC()', we are ok).  So this should be safe.
> 
> There is still the underlying problem that, on the 8260, 'HAL_DCACHE_SYNC()'
> does not guarantee that main memory is coherent with the cache.  I have
> thought about changing the region that 'HAL_DCACHE_SYNC()' loads to be in
> flash rather than SDRAM (and double checking that the flash is marked as
> cacheable), but I would want to check the read timings on that vs. the burst
> read timings of SDRAM to see how loading the two regions compared.  (If it
> takes more than twice as long, then we might as well scan double the number
> of cache lines as in my original proposal -- if it takes less time, then
> we've got a winner).  I have also thought about using one of the chip select
> state machines to make an alias of SDRAM that is only used for syncing the D
> cache (assuming I can get away with that).  All in all, the simplest
> solution is to double the number of cache lines read -- how often does the
> cache need to by sync'd in a real time application?  (RedBoot doesn't count
> here).
> 

I don't see a good way around this either (I've researched what's happening
in the LinuxPPC world as well).  I'd say that the best solution, certainly
the easiest, is just to double the reads (pretend the cache is twice the
size) for the SYNC() operation.  This will always work, based on how the
PowerPC caches function.

Note: there are currently only two places in eCos where this is "used".
Both are rare - FLASH operations (which take forever anyway) and when
booting a Linux kernel (because one normally has to hand off the machine
with the caches off).  Neither of these is time-critical nor repeated
often.

So, Patrick, please resurrect your original patch.  Can you make it
for all PowerPC SYNC() macros as well?

Thanks.
 
> --wpd
> 
> 
> > -----Original Message-----
> > From: Gary Thomas [mailto:gary at mlbassoc dot com]
> > Sent: Tuesday, March 25, 2003 9:50 AM
> > To: Patrick Doyle
> > Cc: eCos patches
> > Subject: RE: MPC8260 cache patch
> >
> >
> > On Tue, 2003-03-25 at 07:35, Patrick Doyle wrote:
> > > In
> > >
> > "packages/io/flash/current/include/flash.hpackages/io/flash/curren
> > t/include/
> > > flash.h", there is the following definition:
> > >
> > > #define HAL_FLASH_CACHES_OFF(_d_, _i_)          \
> > >     CYG_MACRO_START                             \
> > >     _i_ = 0; /* avoids warning */               \
> > >     HAL_DCACHE_IS_ENABLED(_d_);                 \
> > >     HAL_DCACHE_SYNC();                          \
> > >     HAL_DCACHE_INVALIDATE_ALL();                \
> > >     HAL_DCACHE_DISABLE();                       \
> > >     HAL_ICACHE_INVALIDATE_ALL();                \
> > >     CYG_MACRO_END
> > >
> > > This is called in prior to the call to 'flash_dev_query()' (and
> > > 'HAL_FLASH_CACHES_ON()' is called afterwards).  When it is
> > called during the
> > > the boot process of RedBoot, RedBoot has written some values to
> > the virtual
> > > vector table that do not get committed to memory by the
> > (current version of)
> > > 'HAL_DCACHE_SYNC()'.  Then when 'HAL_DCACHE_INVALIDATE_ALL()'
> > is invoked,
> > > those changes are lost.
> > >
> >
> > I don't think that for the purposes of the FLASH code that the
> > invalidates need to be there.  Notice that they are not present in
> > the older version of this macro.
> >
> > Can you try this without the invalidate lines?
> >
> > > You won't get any argument from me about the desire to fully
> > understand a
> > > problem prior to fixing it.  There is a huge difference between fixing a
> > > problem and making it go away.  In my experience, the latter practice is
> > > encountered significantly more often than the former.
> > >
> > > OTOH,
> > >
> > > As I was thinking about this last night, I started to wonder,
> > why does it
> > > matter if the sync time is doubled?  The effeciency expert in
> > my cringes to
> > > hear me write this (huh?) but, seriously, how often does the
> > cache need to
> > > be sync'd in eCos.  We don't perform MMU-style context switches as Linux
> > > does.  The caches on the '8260 support snooping (in theory -- are there
> > > erratta about this not working?) so in an MP system, the
> > hardware will take
> > > care of coherency issues.  As I said, when I first read your
> > reply saying
> > > that you didn't want do double the time it takes to sync the
> > cache, my first
> > > reaction was one of total agreement.  But, as I thought about it more, I
> > > wonder, is it really an issue for eCos?
> > >
> >
> > We do try to keep things as slim and fast as possible; after all that's
> > one of eCos' selling points!  A little here, a little there and soon
> > we'll lose control.  It may be that the only safe way to do this flush
> > is by doubling the lines, but I want to fully investigate first.
> >
> > > --wpd
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gary Thomas [mailto:gary at mlbassoc dot com]
> > > > Sent: Monday, March 24, 2003 6:42 PM
> > > > To: Patrick Doyle
> > > > Cc: eCos patches
> > > > Subject: RE: MPC8260 cache patch
> > > >
> > > >
> > > > On Mon, 2003-03-24 at 15:12, Patrick Doyle wrote:
> > > > > The particular problem I was chasing down was related to the
> > > > fact that the
> > > > > virtual vector table is in the first 16K.  (Perhaps it could be
> > > > moved for
> > > > > the '8620 platform).  RedBoot initializes pointers in there
> > > > that do not get
> > > > > committed to main memory before the flash initialization
> > > > routine invalidates
> > > > > the cache.
> > > > >
> > > >
> > > > I don't understand.  The FLASH routines don't invalidate the
> > cache, only
> > > > flush them.  Then they turn it off while running the code and then
> > > > restore the cache [turn it back on].  There should be no need
> > to access
> > > > any data within that low 16K while the cache is off.  What data that's
> > > > in the cache and not being flushed is being missed?
> > > >
> > > > This all works fine on all other platforms, including some 603 based
> > > > ones.
> > > >
> > > > > The only other solutions I can think of (now that you
> > mention the major
> > > > > problem of doubling the flush time) are to not place any
> > > > writable data in
> > > > > the first 16K or to choose some other 16K region of cacheable
> > > > memory that
> > > > > could be used to flush the cache.
> > > > >
> > > >
> > > > That's how it works on some other platforms.  The StrongARM has a
> > > > special memory region which is used for only this (it's a sink hole).
> > > >
> > > > Note: I'm not saying that we don't need to fix this.  I just want to
> > > > fully understand it first.
> > > >
> > > > --
> > > > ------------------------------------------------------------
> > > > Gary Thomas                 |
> > > > MLB Associates              |  Consulting for the
> > > > +1 (970) 229-1963           |    Embedded world
> > > > http://www.mlbassoc.com/    |
> > > > email: <gary at mlbassoc dot com>  |
> > > > gpg: http://www.chez-thomas.org/gary/gpg_key.asc
> > > > ------------------------------------------------------------
> > > >
> > --
> > ------------------------------------------------------------
> > Gary Thomas                 |
> > MLB Associates              |  Consulting for the
> > +1 (970) 229-1963           |    Embedded world
> > http://www.mlbassoc.com/    |
> > email: <gary at mlbassoc dot com>  |
> > gpg: http://www.chez-thomas.org/gary/gpg_key.asc
> > ------------------------------------------------------------
> >
> ----
> 

> Index: packages/io/flash/current/ChangeLog
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/io/flash/current/ChangeLog,v
> retrieving revision 1.26
> diff -u -5 -p -r1.26 ChangeLog
> --- packages/io/flash/current/ChangeLog	19 Mar 2003 14:14:56 -0000	1.26
> +++ packages/io/flash/current/ChangeLog	27 Mar 2003 18:43:25 -0000
> @@ -1,5 +1,12 @@
> +2003-03-27  Patrick Doyle  <wpd at delcomsys dot com>
> +
> +	* include/flash.h (HAL_FLASH_CACHES_OFF): Removed overzealous
> +	calls to 'HAL_DCACHE_INVALIDATE_ALL()' and
> +	'HAL_ICACHE_INVALIDATE_ALL()'.
> +	
> +
>  2003-03-19  Thomas Koeller <thomas dot koeller at baslerweb dot com>
>  
>  	* src/flashiodev.c: Fixed trivial error.
>  
>  2003-03-03  Knud Woehler <knud dot woehler at microplex dot de>
> Index: packages/io/flash/current/include/flash.h
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/io/flash/current/include/flash.h,v
> retrieving revision 1.15
> diff -u -5 -p -r1.15 flash.h
> --- packages/io/flash/current/include/flash.h	23 May 2002 23:06:14 -0000	1.15
> +++ packages/io/flash/current/include/flash.h	27 Mar 2003 18:43:25 -0000
> @@ -202,13 +202,11 @@ externC cyg_bool plf_flash_query_soft_wp
>  #define HAL_FLASH_CACHES_OFF(_d_, _i_)          \
>      CYG_MACRO_START                             \
>      _i_ = 0; /* avoids warning */               \
>      HAL_DCACHE_IS_ENABLED(_d_);                 \
>      HAL_DCACHE_SYNC();                          \
> -    HAL_DCACHE_INVALIDATE_ALL();                \
>      HAL_DCACHE_DISABLE();                       \
> -    HAL_ICACHE_INVALIDATE_ALL();                \
>      CYG_MACRO_END
>  
>  #define HAL_FLASH_CACHES_ON(_d_, _i_)           \
>      CYG_MACRO_START                             \
>      if (_d_) HAL_DCACHE_ENABLE();               \
-- 
------------------------------------------------------------
Gary Thomas                 |
MLB Associates              |  Consulting for the
+1 (970) 229-1963           |    Embedded world
http://www.mlbassoc.com/    |
email: <gary at mlbassoc dot com>  |
gpg: http://www.chez-thomas.org/gary/gpg_key.asc
------------------------------------------------------------


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