IO FLASH and caches

IO FLASH and caches

[Edgar Grimberg]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

Hi,

The attached patch handles the case where the CPU cannot disable
caches and needs to provide an uncached address instead. The fix was
tested with a NIOS2 CPU.

Regards,
Edgar

-- 
Edgar Grimberg
System Developer
Zylin AS
ZY1000 JTAG Debugger http://www.zylin.com/zy1000.html
Phone: (+47) 51 63 25 00

[-- Attachment #2: io_flash_patch.txt --]
[-- Type: text/plain, Size: 1875 bytes --]

Index: current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/flash/current/ChangeLog,v
retrieving revision 1.53
diff -u -r1.53 ChangeLog
--- current/ChangeLog	1 Jul 2009 18:52:09 -0000	1.53
+++ current/ChangeLog	16 Oct 2009 07:36:48 -0000
@@ -1,3 +1,8 @@
+2009-10-16  Edgar Grimberg <edgar.grimberg@zylin.com>
+
+	* src/flash.c: Handles the case where the CPU cannot disable caches and needs 
+	to provide an uncached address instead
+
 2009-03-02  Sergei Gavrikov  <sergei.gavrikov@gmail.com>
 
 	* doc/flash.sgml: Fixed typos in a FLASH API listing.
Index: current/src/flash.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/flash/current/src/flash.c,v
retrieving revision 1.32
diff -u -r1.32 flash.c
--- current/src/flash.c	20 Feb 2009 22:06:15 -0000	1.32
+++ current/src/flash.c	16 Oct 2009 07:36:48 -0000
@@ -106,6 +106,12 @@
 #define CHECK_SOFT_WRITE_PROTECT(_addr_, _len_) CYG_EMPTY_STATEMENT
 #endif
 
+#ifdef CYGARC_UNCACHED_ADDRESS
+#	define UNCACHED_ADDRESS(_x_) CYGARC_UNCACHED_ADDRESS(_x_)
+#else
+#	define UNCACHED_ADDRESS(_x_) _x_
+#endif
+
 // Has the FLASH IO library been initialised?
 static bool init;
 
@@ -622,10 +628,14 @@
     stat = dev->funs->flash_program(dev, addr, ram, this_write);
 #ifdef CYGSEM_IO_FLASH_VERIFY_PROGRAM
     if (CYG_FLASH_ERR_OK == stat) // Claims to be OK
-      if (!dev->funs->flash_read && memcmp((void *)addr, ram, this_write) != 0) {                
+    {
+	 cyg_flashaddr_t unchached_addr;
+	 unchached_addr = UNCACHED_ADDRESS(addr);
+      if (!dev->funs->flash_read && memcmp((volatile void *)unchached_addr, ram, this_write) != 0) {
         stat = CYG_FLASH_ERR_DRV_VERIFY;
         CHATTER(dev, "V");
       }
+    }
 #endif
     if (CYG_FLASH_ERR_OK != stat) {
         if (err_address)

Re: IO FLASH and caches

[Nick Garnett]

Edgar Grimberg <edgar.grimberg@zylin.com> writes:

> Hi,
> 
> The attached patch handles the case where the CPU cannot disable
> caches and needs to provide an uncached address instead. The fix was
> tested with a NIOS2 CPU.

I don't think this patch is correct. In general flash should be read
through the cache. Flash devices are slow and using the cache improves
performance. It is the responsibility of the flash driver to ensure
that stale data is evicted from the caches by calling cache control
macros. In general the drivers already do this.

This patch forces uncached access for any architecture that defines
CYGARC_UNCACHED_ADDRESS(). Many platforms (e.g. MIPS, PPC, ARM9)
provide both cached and uncached views of memory. These architectures
also provide mechanisms for flushing and invalidating the caches.

As far as I am aware, the NIOS2 has cache control instructions, so I
am not sure what the actual reason for this patch is.

In any case, I don't think that other architectures should be forced
to operate at lower performance just to accommodate the shortcomings
of one particular architecture.

-- 
Nick Garnett                                       eCos Kernel Architect
eCosCentric Limited    http://www.eCosCentric.com       The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales:                         Reg No: 4422071

Re: IO FLASH and caches

[Edgar Grimberg]

Hi Nick,

Thanks for the feedback.

On Fri, Oct 16, 2009 at 1:04 PM, Nick Garnett <nickg@ecoscentric.com> wrote:
> Edgar Grimberg <edgar.grimberg@zylin.com> writes:
>
>> Hi,
>>
>> The attached patch handles the case where the CPU cannot disable
>> caches and needs to provide an uncached address instead. The fix was
>> tested with a NIOS2 CPU.
>
> I don't think this patch is correct. In general flash should be read
> through the cache. Flash devices are slow and using the cache improves
> performance. It is the responsibility of the flash driver to ensure
> that stale data is evicted from the caches by calling cache control
> macros. In general the drivers already do this.
>
> This patch forces uncached access for any architecture that defines
> CYGARC_UNCACHED_ADDRESS(). Many platforms (e.g. MIPS, PPC, ARM9)
> provide both cached and uncached views of memory. These architectures
> also provide mechanisms for flushing and invalidating the caches.

The only place where the uncached address is used is inside the area guarded by:

HAL_FLASH_CACHES_OFF(d_cache, i_cache);
...
HAL_FLASH_CACHES_ON(d_cache, i_cache);

So the patch is not supposed to modify the intended access type.

>
> As far as I am aware, the NIOS2 has cache control instructions, so I
> am not sure what the actual reason for this patch is.

 NIOS2 cannot just turn the caches off. The only way to say that a
memory range should not be accessed via caches is to set the MSB bit
of the address to 1.

> In any case, I don't think that other architectures should be forced
> to operate at lower performance just to accommodate the shortcomings
> of one particular architecture.

Agree. I'm not sure that's the case, though.

Regards,
Edgar

>
> --
> Nick Garnett                                       eCos Kernel Architect
> eCosCentric Limited    http://www.eCosCentric.com       The eCos experts
> Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
> Registered in England and Wales:                         Reg No: 4422071
>
>



-- 
Edgar Grimberg
System Developer
Zylin AS
ZY1000 JTAG Debugger http://www.zylin.com/zy1000.html
Phone: (+47) 51 63 25 00

Re: IO FLASH and caches

[Nick Garnett]

Edgar Grimberg <edgar.grimberg@zylin.com> writes:

> The only place where the uncached address is used is inside the area guarded by:
> 
> HAL_FLASH_CACHES_OFF(d_cache, i_cache);
> ...
> HAL_FLASH_CACHES_ON(d_cache, i_cache);
> 
> So the patch is not supposed to modify the intended access type.


OK. My quick look at the patch and the code missed that.

However, I'm stll not sure that it is necessary to change this code.
The call to the device flash_program() routine will still evict the
caches and this verification will just refill the cache with the new
flash contents. Functionally, it should remain the same. We have never
needed to make this change for other targets that have similar cache
behavior, so I'm still unclear what the issue here is.



-- 
Nick Garnett                                       eCos Kernel Architect
eCosCentric Limited    http://www.eCosCentric.com       The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales:                         Reg No: 4422071

Re: IO FLASH and caches

[Bart Veer]

    >>> The attached patch handles the case where the CPU cannot
    >>> disable caches and needs to provide an uncached address
    >>> instead. The fix was tested with a NIOS2 CPU.
    >> 
    >> I don't think this patch is correct. In general flash should be
    >> read through the cache. Flash devices are slow and using the
    >> cache improves performance. It is the responsibility of the
    >> flash driver to ensure that stale data is evicted from the
    >> caches by calling cache control macros. In general the drivers
    >> already do this.
    >> 
    >> This patch forces uncached access for any architecture that
    >> defines CYGARC_UNCACHED_ADDRESS(). Many platforms (e.g. MIPS,
    >> PPC, ARM9) provide both cached and uncached views of memory.
    >> These architectures also provide mechanisms for flushing and
    >> invalidating the caches.

I think the patch may be fundamentally flawed.

If I understand correctly, the issue you are trying to address is that
after a flash program operation that involves bypassing the cache, the
data cache may still contain stale data from before. So you change the
verify code to look at the uncached flash contents. The problem is
that any subsequent code reading the flash may still get the stale
data from the cache.

In other words, you have not solved the problem. All that the patch
does is stop the verify code from reporting the problem.

What should happen is that the flash driver does whatever is necessary
to keep the cache and the flash contents coherent. I believe the V2
AMD and Strata drivers should do the right thing. _V2_CACHED_ONLY
should not be defined because the Nios II can bypass the cache when
manipulating flash, so _INTSCACHE_DEFAULT_END() should sync and
invalidate the data cache. When control returns to the generic flash
code all stale data should have been removed from the cache so the
verify code will see the new data.

It is possible that some of the V1 drivers get this wrong. If so, the
solution is to switch to a V2 driver.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.

Re: IO FLASH and caches

[Edgar Grimberg]

> However, I'm stll not sure that it is necessary to change this code.
> The call to the device flash_program() routine will still evict the
> caches and this verification will just refill the cache with the new
> flash contents. Functionally, it should remain the same. We have never
> needed to make this change for other targets that have similar cache
> behavior, so I'm still unclear what the issue here is.

I'm trying to add a STRATA v2 driver in my code. Reading from
http://ecos.sourceware.org/docs-latest/ref/strata-instance.html#STRATA-INSTANCE-CACHE
I got the impression that my first choice is to implement the
CYGARC_UNCACHED_ADDRESS in the HAL and I'm off the hook. All the other
solutions, including defining STRATA_INTSCACHE_BEGIN and
STRATA_INTSCACHE_END looked like workarounds if I cannot bypass caches
using the address trick.
The problem starts when I don't invalidate the cache in the STRATA
driver. The verification in IO FLASH fails, and this is the first bad
thing that happened. That was also the first thing I tried to fix.
I guess I should make a patch for the STRATA v2 documentation and add
a cache invalidate in the  HAL_STRATA_INTSCACHE_END macro in my HAL.


-- 
Edgar Grimberg
System Developer
Zylin AS
ZY1000 JTAG Debugger http://www.zylin.com/zy1000.html
Phone: (+47) 51 63 25 00

Re: IO FLASH and caches

[Edgar Grimberg]

> I think the patch may be fundamentally flawed.

Yes, I also believe the problem lies somewhere else.

>
> If I understand correctly, the issue you are trying to address is that
> after a flash program operation that involves bypassing the cache, the
> data cache may still contain stale data from before. So you change the
> verify code to look at the uncached flash contents. The problem is
> that any subsequent code reading the flash may still get the stale
> data from the cache.

That's spot on.

> In other words, you have not solved the problem. All that the patch
> does is stop the verify code from reporting the problem.

Yes, I'm afraid that's the case.

> What should happen is that the flash driver does whatever is necessary
> to keep the cache and the flash contents coherent. I believe the V2
> AMD and Strata drivers should do the right thing. _V2_CACHED_ONLY
> should not be defined because the Nios II can bypass the cache when
> manipulating flash, so _INTSCACHE_DEFAULT_END() should sync and
> invalidate the data cache. When control returns to the generic flash
> code all stale data should have been removed from the cache so the
> verify code will see the new data.

I am running STRATA V2.
From documentation I've got the impression that, if I can bypass the
caches by modifying the address, I don't need the
_INTSCACHE_DEFAULT_END macro. I guess this is wrong.

Thanks for helping.

Edgar

-- 
Edgar Grimberg
System Developer
Zylin AS
ZY1000 JTAG Debugger http://www.zylin.com/zy1000.html
Phone: (+47) 51 63 25 00

Re: IO FLASH and caches

[Bart Veer]

>>>>> "Edgar" == Edgar Grimberg <edgar.grimberg@zylin.com> writes:

    >> What should happen is that the flash driver does whatever is
    >> necessary to keep the cache and the flash contents coherent. I
    >> believe the V2 AMD and Strata drivers should do the right
    >> thing. _V2_CACHED_ONLY should not be defined because the Nios
    >> II can bypass the cache when manipulating flash, so
    >> _INTSCACHE_DEFAULT_END() should sync and invalidate the data
    >> cache. When control returns to the generic flash code all stale
    >> data should have been removed from the cache so the verify code
    >> will see the new data.

    Edgar> I am running STRATA V2. From documentation I've got the
    Edgar> impression that, if I can bypass the caches by modifying
    Edgar> the address, I don't need the _INTSCACHE_DEFAULT_END macro.
    Edgar> I guess this is wrong.

If your HAL provides HAL_UNCACHED_ADDRESS() and does not implement the
CDL interface _V2_CACHED_ONLY then the AMD and V2 drivers should
provide sensible implementations of the _INTSCACHE_ macros themselves.
See the driver code for more details. For maximum flexibility the HALs
can provide alternative implementations of these macros, but that
should only be necessary in exceptional cases.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.

Re: IO FLASH and caches

[Edgar Grimberg]

> If your HAL provides HAL_UNCACHED_ADDRESS() and does not implement the
> CDL interface _V2_CACHED_ONLY then the AMD and V2 drivers should
> provide sensible implementations of the _INTSCACHE_ macros themselves.
> See the driver code for more details. For maximum flexibility the HALs
> can provide alternative implementations of these macros, but that
> should only be necessary in exceptional cases.

I was looking at the entire driver code and I got the picture. Thanks
for pointing into the right direction.
As a result, it looks like my HAL_DCACHE_INVALIDATE_ALL is broken and
it was hard to catch it red handed.

Edgar

>
> Bart
>
> --
> Bart Veer                                   eCos Configuration Architect
> eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
> Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
> Registered in England and Wales: Reg No 4422071.
>



-- 
Edgar Grimberg
System Developer
Zylin AS
ZY1000 JTAG Debugger http://www.zylin.com/zy1000.html
Phone: (+47) 51 63 25 00