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 Fri, 2003-03-28 at 07:20, Patrick Doyle wrote:
> 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?
> 

Yes, I also think that it's safe in this case.

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