This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
RE: MPC8260 cache patch
- From: "Patrick Doyle" <wpd at delcomsys dot com>
- To: "Gary Thomas" <gary at mlbassoc dot com>
- Cc: "eCos patches" <ecos-patches at sources dot redhat dot com>
- Date: Fri, 28 Mar 2003 09:20:43 -0500
- Subject: RE: MPC8260 cache patch
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
> ------------------------------------------------------------
>