RE: MPC8260 cache patch

["Patrick Doyle" <wpd at delcomsys dot com>]

OK, I'll try to put it together over the weekend and send out a patch
Monday.

Just as a sanity check, do you think that removing the 'INVALIDATE()' calls
from the 'HAL_FLASH_CACHES_OFF()' macro is still worth doing?  I think it
should be safe (note the big difference between "I think" and "I have proved
conclusively" here).  Or do you think we should leave them in?

--wpd


> -----Original Message-----
> From: Gary Thomas [mailto:gary at mlbassoc dot com]
> Sent: Friday, March 28, 2003 7:35 AM
> To: Patrick Doyle
> Cc: eCos patches
> Subject: 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
> ------------------------------------------------------------
>