i.MX51: FEC: Cache coherency problem?

Hi all,

I am busy debugging a problem with the i.MX51 FEC ethernet driver, that stopped working after upgrading u-boot. Before the upgrade I used v2010.06-rc3, which worked fine.
I gave up on trying to find the difference beween the old version and this one that broke it.... due to BSP issues, git bisecting seems a monumental task I am not considering yet.
The funny part is that it seems to work fine if I disable data-caches! With data caches enabled, it hangs in the following while loop in fec_send(), at line 592:

...
584 /*
585 * Enable SmartDMA transmit task
586 */
587 fec_tx_task_enable(fec);
588
589 /*
590 * wait until frame is sent .
591 */
592 while (readw(&fec->tbd_base[fec->tbd_index].status)& FEC_TBD_READY) {
593 udelay(1);
594 }
...

If I change this code in the following way, the while loop exits successfully:

...
584 /*
585 * Enable SmartDMA transmit task
586 */

587         flush_cache(&fec->tbd_base[fec->tbd_index], 4);
588         fec_tx_task_enable(fec);
589         flash_dcache_all();

590
591 /*
592 * wait until frame is sent .
593 */
594 while (readw(&fec->tbd_base[fec->tbd_index].status)& FEC_TBD_READY) {
595 udelay(1);
596 }
...

Notice the cache flush calls at line 587 and 589. With these, sending succeeds. The driver still hangs in receiving afterwards, but at least this part seems to work. If I remove either of the two added lines, it stops working again.

What is going on here? Why did this work with caches enabled before??

Hi David,

Quoted 51 lines · David Jander

Hi all,

I am busy debugging a problem with the i.MX51 FEC ethernet driver, that stopped working after upgrading u-boot. Before the upgrade I used v2010.06-rc3, which worked fine.
I gave up on trying to find the difference beween the old version and this one that broke it.... due to BSP issues, git bisecting seems a monumental task I am not considering yet.
The funny part is that it seems to work fine if I disable data-caches! With data caches enabled, it hangs in the following while loop in fec_send(), at line 592:

...
584 /*
585 * Enable SmartDMA transmit task
586 */
587 fec_tx_task_enable(fec);
588
589 /*
590 * wait until frame is sent .
591 */
592 while (readw(&fec->tbd_base[fec->tbd_index].status)&
FEC_TBD_READY) { 593 udelay(1);
594 }
...

If I change this code in the following way, the while loop exits successfully:

...
584 /*
585 * Enable SmartDMA transmit task
586 */

587         flush_cache(&fec->tbd_base[fec->tbd_index], 4);
588         fec_tx_task_enable(fec);
589         flash_dcache_all();

590
591 /*
592 * wait until frame is sent .
593 */
594 while (readw(&fec->tbd_base[fec->tbd_index].status)&
FEC_TBD_READY) { 595 udelay(1);
596 }
...

Notice the cache flush calls at line 587 and 589. With these, sending succeeds. The driver still hangs in receiving afterwards, but at least this part seems to work. If I remove either of the two added lines, it stops working again.

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

Are you sure caches were enabled before? Until recently absence of CONFIG_SYS_DCACHE_OFF did not mean that your D-cache is enabled because you also had to call the function dcache_enable(). Some platforms were calling this from board file or some drivers but not all.

The following patch changed this for ARM:

commit c2dd0d45540397704de9b13287417d21049d34c6
armv7: integrate cache maintenance support

In this patch I added a call to dcache_enable() at the beginning of board_init_r() for ARM(i.e. as soon as relocation is over). As a result D-cache will be enabled for all ARM platforms now unless CONFIG_SYS_DCACHE_OFF is set.

Looks like this is a real coherency issue that is brought out because d-cache is really enabled for you now.

commit c2dd0d45540397704de9b13287417d21049d34c6
armv7: integrate cache maintenance support

In this patch I added a call to dcache_enable() at the beginning of board_init_r() for ARM(i.e. as soon as relocation is over). As a result D-cache will be enabled for all ARM platforms now unless CONFIG_SYS_DCACHE_OFF is set.

Dear Aneesh

Am 18.07.2011 18:16, schrieb Aneesh V:

Quoted 7 lines

commit c2dd0d45540397704de9b13287417d21049d34c6
armv7: integrate cache maintenance support

In this patch I added a call to dcache_enable() at the beginning of board_init_r() for ARM(i.e. as soon as relocation is over). As a result D-cache will be enabled for all ARM platforms now unless CONFIG_SYS_DCACHE_OFF is set.

Is this really a good idea? This will break a couple of boards using non-cache-aware drivers. And there are a couple of them in u-boot. I think d-cache should be opt-in rather then opt-out as long as there are any drivers which didn't handle cached memory regions correct. i-cache is much less problematic and can be enabled by default.

If d-cache will be enabled by default on ARM I think I have to send a patch for one of my boards :-)

Now I finally know what's wrong and am working on a proposed fix to make this one driver cache-aware.

Dear David Jander,

Quoted 3 lines · In message <20110719131744.403a81e6@archvile> you wrote:

Now I finally know what's wrong and am working on a proposed fix to make this one driver cache-aware.

I would just like to point out that these efforts are highly appreciated!

Quoted 9 lines · On Tue, 19 Jul 2011 13:20:26 +0200 Wolfgang Denk <[email protected]> wrote:

Dear David Jander,

Quoted 3 lines · In message <20110719131744.403a81e6@archvile> you wrote:

Now I finally know what's wrong and am working on a proposed fix to make this one driver cache-aware.

I would just like to point out that these efforts are highly appreciated!

Thanks.
I'll try my best, but I am running out of time, and it is still not as it should. I still have trouble identifying the right place where receive buffer descriptors should be cache-invalidated. At one point, a magic flush_dcache_all() at a certain place in code made it work, but that can't be entirely correct, and if I replace it with only the necessary ranges, it doesn't work correctly anymore, so I guess this flush is also invalidating something I need elsewhere :-(
I keep searching.... through the IMO fairly convoluted network code.

Quoted 13 lines · On Tue, 19 Jul 2011 14:10:48 +0200 David Jander<[email protected]>…

Quoted 9 lines · On Tue, 19 Jul 2011 13:20:26 +0200 Wolfgang Denk<[email protected]> wrote:

Dear David Jander,

Quoted 3 lines · In message<20110719131744.403a81e6@archvile> you wrote:

Now I finally know what's wrong and am working on a proposed fix to make this one driver cache-aware.

I would just like to point out that these efforts are highly appreciated!

Quoted 9 lines

Thanks.
I'll try my best, but I am running out of time, and it is still not as it should. I still have trouble identifying the right place where receive buffer descriptors should be cache-invalidated. At one point, a magic flush_dcache_all() at a certain place in code made it work, but that can't be entirely correct, and if I replace it with only the necessary ranges, it doesn't work correctly anymore, so I guess this flush is also invalidating something I need elsewhere :-(
I keep searching.... through the IMO fairly convoluted network code.

Ok, that was not such a problem. I found few nice spots, where cache-maintenance code should go IMO:

In fec_rx_task_enable() cleaned RX BD's should be flushed.
In fec_tx_task_enable() dirty TX BD's should be flushed, as well as data buffers.
In fec_send() Invalidate BD status fields before reading, also in the while loop.
In fec_recv() Invalidate BD and data buffers just before every read.

And of course align all buffers to cache-line size and pad around the end of each buffer to cache-line size.

Problem: It doesn't work (tftp transfer stops after a few packets)! :-( I am giving up!

Plan B: I'll now try to allocate all buffers in internal RAM (which is not cached AFAIK).

Any ideas?

Best regards,

Hi David,

Le 20/07/2011 08:29, David Jander a ?crit :

Quoted 39 lines

Quoted 13 lines · On Tue, 19 Jul 2011 14:10:48 +0200 David Jander<[email protected]>…

Quoted 9 lines · On Tue, 19 Jul 2011 13:20:26 +0200 Wolfgang Denk<[email protected]> wrote:

Dear David Jander,

Quoted 3 lines · In message<20110719131744.403a81e6@archvile> you wrote:

Now I finally know what's wrong and am working on a proposed fix to make this one driver cache-aware.

I would just like to point out that these efforts are highly appreciated!

Quoted 9 lines

Thanks.
I'll try my best, but I am running out of time, and it is still not as it should. I still have trouble identifying the right place where receive buffer descriptors should be cache-invalidated. At one point, a magic flush_dcache_all() at a certain place in code made it work, but that can't be entirely correct, and if I replace it with only the necessary ranges, it doesn't work correctly anymore, so I guess this flush is also invalidating something I need elsewhere :-(
I keep searching.... through the IMO fairly convoluted network code.

Ok, that was not such a problem. I found few nice spots, where cache-maintenance code should go IMO:

In fec_rx_task_enable() cleaned RX BD's should be flushed.
In fec_tx_task_enable() dirty TX BD's should be flushed, as well as data buffers.
In fec_send() Invalidate BD status fields before reading, also in the while loop.
In fec_recv() Invalidate BD and data buffers just before every read.

And of course align all buffers to cache-line size and pad around the end of each buffer to cache-line size.

Quoted 7 lines

Problem: It doesn't work (tftp transfer stops after a few packets)! :-( I am giving up!

Plan B: I'll now try to allocate all buffers in internal RAM (which is not cached AFAIK).

Any ideas?

Yes, one: I had issues with the Marvell Ethernet adapter, which has DMA as well, not because of cache (it was not active at the time) but because of instruction reordering done by the compiler when optimizing, which resulted in out-of-order accesses to the descritpors and DMA registered, so that the DMA start was written before the descriptors were set up. The (proper and clean) solution was to introduce memory barriers at strategic points of the driver to ensure that specific DMA starts were done only after all writes to the descriptors (and possibly buffers).

On Wed, 20 Jul 2011 10:56:07 +0200

Quoted 11 lines

Quoted 1 line

Any ideas?

Yes, one: I had issues with the Marvell Ethernet adapter, which has DMA as well, not because of cache (it was not active at the time) but because of instruction reordering done by the compiler when optimizing, which resulted in out-of-order accesses to the descritpors and DMA registered, so that the DMA start was written before the descriptors were set up. The (proper and clean) solution was to introduce memory barriers at strategic points of the driver to ensure that specific DMA starts were done only after all writes to the descriptors (and possibly buffers).

Hmmm. I know that issue, but AFAICS, the driver already uses readX() and writeX() macros when accessing register and DMA memory. Those macros have compiler barriers included.... and armv7 is still in-order execution ;-)

Quoted 1 line · David Jander

On Wed, 20 Jul 2011 10:56:07 +0200

[snip ..]

Quoted 12 lines

Quoted 11 lines

Quoted 1 line

Any ideas?

Yes, one: I had issues with the Marvell Ethernet adapter, which has DMA as well, not because of cache (it was not active at the time) but because of instruction reordering done by the compiler when optimizing, which resulted in out-of-order accesses to the descritpors and DMA registered, so that the DMA start was written before the descriptors were set up. The (proper and clean) solution was to introduce memory barriers at strategic points of the driver to ensure that specific DMA starts were done only after all writes to the descriptors (and possibly buffers).

Quoted 3 lines

Hmmm. I know that issue, but AFAICS, the driver already uses readX() and writeX() macros when accessing register and DMA memory. Those macros have compiler barriers included.... and armv7 is still in-order execution ;-)

1. armv7 is not necessarily in-order. Cortex-A8 is in-order while Cortex-A9 is out-of-order.
2. I am not sure if in-order execution guarantees memory ordering. My understanding is that it doesn't.
3. In u-boot's implementation of MMU for ARM, the register space(everything other than SDRAM) is 'strongly-ordered' memory. Strongly ordered accesses will be correctly ordered w.r.to all other accesses in program order.

4. Compiler barriers prevent the compiler from doing certain optimizations, but doesn't help in these situations.

In short, I think you don't need to worry about ordering, but not because of compiler barriers or armv7 being in-order.

Hi Aneesh,

Quoted 21 lines · On Wed, 20 Jul 2011 15:59:42 +0530 Aneesh V<[email protected]> wrote:

Quoted 1 line · David Jander

On Wed, 20 Jul 2011 10:56:07 +0200

[snip ..]

Quoted 12 lines

Quoted 11 lines

Quoted 1 line

Any ideas?

Yes, one: I had issues with the Marvell Ethernet adapter, which has DMA as well, not because of cache (it was not active at the time) but because of instruction reordering done by the compiler when optimizing, which resulted in out-of-order accesses to the descritpors and DMA registered, so that the DMA start was written before the descriptors were set up. The (proper and clean) solution was to introduce memory barriers at strategic points of the driver to ensure that specific DMA starts were done only after all writes to the descriptors (and possibly buffers).

Quoted 3 lines

Hmmm. I know that issue, but AFAICS, the driver already uses readX() and writeX() macros when accessing register and DMA memory. Those macros have compiler barriers included.... and armv7 is still in-order execution ;-)

Quoted 8 lines

1. armv7 is not necessarily in-order. Cortex-A8 is in-order while Cortex-A9 is out-of-order.
2. I am not sure if in-order execution guarantees memory ordering. My understanding is that it doesn't.
3. In u-boot's implementation of MMU for ARM, the register space(everything other than SDRAM) is 'strongly-ordered' memory. Strongly ordered accesses will be correctly ordered w.r.to all other accesses in program order.

Wow, I hadn't noticed dcache_enable() also enables the MMU. Cool. One step closer to splitting the mapping between a cached and uncached, strongly-ordered region of RAM.

Quoted 3 lines

4. Compiler barriers prevent the compiler from doing certain optimizations, but doesn't help in these situations.

Thanks for this excellent summary!

Quoted 2 lines

In short, I think you don't need to worry about ordering, but not because of compiler barriers or armv7 being in-order.

Well, I do need to worry about the buffer-descriptors and buffers then, since they are in SDRAM, right?

Is this really a good idea? This will break a couple of boards using non-cache-aware drivers. And there are a couple of them in u-boot. I think d-cache should be opt-in rather then opt-out as long as there are any drivers which didn't handle cached memory regions correct. i-cache is much less problematic and can be enabled by default.

If d-cache will be enabled by default on ARM I think I have to send a patch for one of my boards :-)

Dear =?ISO-8859-1?Q?Matthias_Wei=DFer?=,

Quoted 1 line · In message<[email protected]> you wrote:

Quoted 5 lines

Is this really a good idea? This will break a couple of boards using non-cache-aware drivers. And there are a couple of them in u-boot. I think d-cache should be opt-in rather then opt-out as long as there are any drivers which didn't handle cached memory regions correct. i-cache is much less problematic and can be enabled by default.

If we do this, then everybody will just be lazy, and nothing will ever change.

Quoted 2 lines

If d-cache will be enabled by default on ARM I think I have to send a patch for one of my boards :-)

Why don't you just help identifying and fixing the problems in the misbehaving drivers?!?

That would be a _much_ more helpful approach.

Dear Aneesh

Am 18.07.2011 18:16, schrieb Aneesh V:

Quoted 7 lines

commit c2dd0d45540397704de9b13287417d21049d34c6
armv7: integrate cache maintenance support

In this patch I added a call to dcache_enable() at the beginning of board_init_r() for ARM(i.e. as soon as relocation is over). As a result D-cache will be enabled for all ARM platforms now unless CONFIG_SYS_DCACHE_OFF is set.

Is this really a good idea? This will break a couple of boards using non-cache-aware drivers. And there are a couple of them in u-boot. I think d-cache should be opt-in rather then opt-out as long as there are any drivers which didn't handle cached memory regions correct. i-cache is much less problematic and can be enabled by default.

Hi all,

What is going on here? Why did this work with caches enabled before??

Quoted 2 lines · David Jander

Hi all,

Hi David,

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

I think cache was always disabled..

Hi Stefano,

Quoted 9 lines · On Mon, 18 Jul 2011 18:55:05 +0200 Stefano Babic<[email protected]> wrote:

Quoted 2 lines · David Jander

Hi all,

Hi David,

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

I think cache was always disabled..

I had even L2-caches enabled in u-boot (copied/adapted some code from OMAP cache.S), and called i/dcache_enable() from board code like this:

int board_late_init(void)

{
         power_init();
         probe_board_type();
         icache_enable();
         dcache_enable();

         return 0;
}

Is there a reason this wouldn't have worked before?

Suppose it didn't. Does that mean we need to use the MMU to properly mark regions of register space and specially FEC BD's as not-cached? Or do we need to flash caches manually each time such a memory region is accessed? I am kind of a CPU-speed-junkie, so I am not sure I want to live without caches enabled in u-boot ;-)

Best regards,

Hi David,

Le 19/07/2011 09:44, David Jander a ?crit :

Quoted 29 lines

Hi Stefano,

Quoted 9 lines · On Mon, 18 Jul 2011 18:55:05 +0200 Stefano Babic<[email protected]> wrote:

Quoted 2 lines · David Jander

Hi all,

Hi David,

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

I think cache was always disabled..

I had even L2-caches enabled in u-boot (copied/adapted some code from OMAP cache.S), and called i/dcache_enable() from board code like this:

int board_late_init(void)

{
         power_init();
         probe_board_type();
         icache_enable();
         dcache_enable();

         return 0;
}

Quoted 7 lines

Is there a reason this wouldn't have worked before?

Suppose it didn't. Does that mean we need to use the MMU to properly mark regions of register space and specially FEC BD's as not-cached? Or do we need to flash caches manually each time such a memory region is accessed? I am kind of a CPU-speed-junkie, so I am not sure I want to live without caches enabled in u-boot ;-)

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

Quoted 32 lines · On Tue, 19 Jul 2011 10:21:12 +0200 Albert ARIBAUD<[email protected]>…

Hi David,

Le 19/07/2011 09:44, David Jander a ?crit :

Quoted 29 lines

Hi Stefano,

Quoted 9 lines · On Mon, 18 Jul 2011 18:55:05 +0200 Stefano Babic<[email protected]> wrote:

Quoted 2 lines · David Jander

Hi all,

Hi David,

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

I think cache was always disabled..

I had even L2-caches enabled in u-boot (copied/adapted some code from OMAP cache.S), and called i/dcache_enable() from board code like this:

int board_late_init(void)

{
          power_init();
          probe_board_type();
          icache_enable();
          dcache_enable();

          return 0;
}

Quoted 7 lines

Quoted 7 lines

Is there a reason this wouldn't have worked before?

Suppose it didn't. Does that mean we need to use the MMU to properly mark regions of register space and specially FEC BD's as not-cached? Or do we need to flash caches manually each time such a memory region is accessed? I am kind of a CPU-speed-junkie, so I am not sure I want to live without caches enabled in u-boot ;-)

Quoted 6 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a correct way of solving this problem?

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

588 fec_tx_task_enable(fec);
589 flush_dcache_all();

590
591 /*
592 * wait until frame is sent .
593 */
594 while (readw(&fec->tbd_base[fec->tbd_index].status)& FEC_TBD_READY) {
595 udelay(1);
596 }

I am still not sure why I need line 587 above.

Quoted 35 lines · David Jander

Quoted 32 lines · On Tue, 19 Jul 2011 10:21:12 +0200 Albert ARIBAUD<[email protected]>…

Hi David,

Le 19/07/2011 09:44, David Jander a ?crit :

Quoted 29 lines

Hi Stefano,

Quoted 9 lines · On Mon, 18 Jul 2011 18:55:05 +0200 Stefano Babic<[email protected]> wrote:

Quoted 2 lines · David Jander

Hi all,

Hi David,

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

I think cache was always disabled..

I had even L2-caches enabled in u-boot (copied/adapted some code from OMAP cache.S), and called i/dcache_enable() from board code like this:

int board_late_init(void)

{
          power_init();
          probe_board_type();
          icache_enable();
          dcache_enable();

          return 0;
}

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Is there a reason this wouldn't have worked before?

Suppose it didn't. Does that mean we need to use the MMU to properly mark regions of register space and specially FEC BD's as not-cached? Or do we need to flash caches manually each time such a memory region is accessed? I am kind of a CPU-speed-junkie, so I am not sure I want to live without caches enabled in u-boot ;-)

Are you talking about some memory-mapped IO region(register space). If that is the case, that region won't be cached. ARM mmu implementation makes only the SDRAM region cached. Rest is non-cached non-buffered.

Quoted 9 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a correct way of solving this problem?

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

Also, please check if flush_cache() is correctly supported for your CPU. The default implementation in in arch/arm/lib/cache.c has support for only a handful of cpus. AFAIK, only armv7 is over-riding this default implementation at the moment.

The fact that it's helping you indicates that it may be working for you. But still worth a check.

Quoted 2 lines

588 fec_tx_task_enable(fec);
589 flush_dcache_all();

This should not be needed.

Quoted 9 lines

590
591 /*
592 * wait until frame is sent .
593 */
594 while (readw(&fec->tbd_base[fec->tbd_index].status)&
FEC_TBD_READY) {
595 udelay(1);
596 }

Quoted 1 line

I am still not sure why I need line 587 above.

Did you try keeping 587 and removing 589?

Quoted 4 lines

Quoted 1 line

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

No it isn't. I know, I should flush the whole buffer area, but this was just enough to get the status field flushed, so the FEC started transmitting, and the while loop ended eventually. The result was still not correct, but at least it won't hang.

What would be more expensive, flushing just the buffer area, or flush_dcache_all()?

Yes, I did. These two lines really is the minimum necessary to exit the while loop. It won't work if I leave out either of those.
Now that I know a little more, I guess this is because of the status field in the buffer-descriptor being checked in the while loop, and that is still in cache. So the only thing line 589 does, is invalidate the caches, so the next readw() returns the value stored by the FEC, which is apparently faster than this piece of code :-)

Best regards,

Hi David,

Le 19/07/2011 10:58, David Jander a ?crit :

Quoted 11 lines

Quoted 4 lines

Quoted 1 line

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

No it isn't. I know, I should flush the whole buffer area, but this was just
enough to get the status field flushed, so the FEC started transmitting, and
the while loop ended eventually. The result was still not correct, but at least it won't hang.

Seems like you're flushing while the DMA engine is running... That's calling for race conditions IMO. You should only have the DMA engine running while you are sending something or while you are expecting to receive something (U-Boot differs from an OS in that devices are not kept running unless actually needed).

Quoted 2 lines

What would be more expensive, flushing just the buffer area, or flush_dcache_all()?

You should call the API for flushing the area only. Obviously, you'll flush either just as much, or possibly more (thus take longer) if you flush the whole data cache.

Dear Aneesh,

Thanks a lot for your replies.

Quoted 44 lines · On Tue, 19 Jul 2011 14:13:34 +0530 Aneesh V<[email protected]> wrote:

Quoted 35 lines · David Jander

Quoted 32 lines · On Tue, 19 Jul 2011 10:21:12 +0200 Albert ARIBAUD<[email protected]>…

Hi David,

Le 19/07/2011 09:44, David Jander a ?crit :

Quoted 29 lines

Hi Stefano,

Quoted 9 lines · On Mon, 18 Jul 2011 18:55:05 +0200 Stefano Babic<[email protected]> wrote:

Quoted 2 lines · David Jander

Hi all,

Hi David,

Quoted 1 line

What is going on here? Why did this work with caches enabled before??

I think cache was always disabled..

I had even L2-caches enabled in u-boot (copied/adapted some code from OMAP cache.S), and called i/dcache_enable() from board code like this:

int board_late_init(void)

{
           power_init();
           probe_board_type();
           icache_enable();
           dcache_enable();

           return 0;
}

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Is there a reason this wouldn't have worked before?

Suppose it didn't. Does that mean we need to use the MMU to properly mark regions of register space and specially FEC BD's as not-cached? Or do we need to flash caches manually each time such a memory region is accessed? I am kind of a CPU-speed-junkie, so I am not sure I want to live without caches enabled in u-boot ;-)

Quoted 3 lines

Are you talking about some memory-mapped IO region(register space). If that is the case, that region won't be cached. ARM mmu implementation makes only the SDRAM region cached. Rest is non-cached non-buffered.

Ah, thanks for pointing out. I already suspected something like that...

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a correct way of solving this problem?

Quoted 6 lines

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

No it isn't. I know, I should flush the whole buffer area, but this was just enough to get the status field flushed, so the FEC started transmitting, and the while loop ended eventually. The result was still not correct, but at least it won't hang.
What would be more expensive, flushing just the buffer area, or flush_dcache_all()?

Quoted 5 lines

Also, please check if flush_cache() is correctly supported for your CPU. The default implementation in in arch/arm/lib/cache.c has support for only a handful of cpus. AFAIK, only armv7 is over-riding this default implementation at the moment.

There is cache_v7.c which implements these... they are supposed to work correctly I guess?

Quoted 10 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a
correct way of solving this problem?

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

Le 19/07/2011 10:43, Aneesh V a ?crit :

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a
correct way of solving this problem?

Quoted 5 lines

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

Generally:

- for sending data through a device that has its own, external, DMA engine, you'll obviously need to flush the data buffer(s) but also any DMA descriptors used by the engine, before you start the engine;

- for rceiving, if you have to set up receive descriptors, you must flush that before telling the device to enter receive mode (so that the device reads the descriptors as you wrote them), and you should invalidate the receive buffers at the latest when the device signals that data has been received,

or preferably long before (at the same time
you flushed the read descriptor, so that cache-related actions are grouped in the same place in the code).

Amicalement,

Quoted 22 lines · Albert ARIBAUD

Le 19/07/2011 10:43, Aneesh V a ?crit :

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a
correct way of solving this problem?

Quoted 5 lines

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

Generally:

- for sending data through a device that has its own, external, DMA engine, you'll obviously need to flush the data buffer(s) but also any DMA descriptors used by the engine, before you start the engine;

Quoted 5 lines

- for rceiving, if you have to set up receive descriptors, you must flush that before telling the device to enter receive mode (so that the device reads the descriptors as you wrote them), and you should invalidate the receive buffers at the latest when the device signals that data has been received,

Hi All,

Quoted 3 lines

or preferably long before (at the same time
you flushed the read descriptor, so that cache-related actions are grouped in the same place in the code).

I think this last statement is incorrect. You should invalidate the cache for the receive buffers just before you intend to reference them. If you do it right after receive mode is entered, subsequent access to items NEAR the receive buffer may reload the cache with receive buffer data before the dma is done, re-creating the problem you are trying to avoid. Also, I don't know if ARM cache is write-back or write -thru, but if it is write-back, the only way to avoid problems is to allocate the receive buffers on cache line boundaries, so no "nearby" writes can cause something in the DMA buffer to be corrupted. If all receive buffers are allocated on cache-line boundaries (both start and end of each buffer), you can invalidate the cache "early" under the assumption that there will be no read accesses to the read buffers themselves until after DMA is complete. IMHO it is better, even in this case., to invalidate cache after dma is done but before referencing the read data.

Quoted 22 lines · Albert ARIBAUD

Le 19/07/2011 10:43, Aneesh V a ?crit :

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a
correct way of solving this problem?

Quoted 5 lines

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

Generally:

- for sending data through a device that has its own, external, DMA engine, you'll obviously need to flush the data buffer(s) but also any DMA descriptors used by the engine, before you start the engine;

Quoted 5 lines

- for rceiving, if you have to set up receive descriptors, you must flush that before telling the device to enter receive mode (so that the device reads the descriptors as you wrote them), and you should invalidate the receive buffers at the latest when the device signals that data has been received,

Hi All,

Quoted 3 lines

? or preferably long before (at the same time
you flushed the read descriptor, so that cache-related actions are grouped in the same place in the code).

I think this last statement is incorrect. You should invalidate the cache for the receive buffers just before you intend to reference them. If you do it right after receive mode is entered, subsequent access to items NEAR the receive buffer may reload the cache with receive buffer data before the dma is done, re-creating the problem you are trying to avoid. Also, I don't know if ARM cache is write-back or write -thru, but if it is write-back, the only way to avoid problems is to allocate the receive buffers on cache line boundaries, so no "nearby" writes can cause something in the DMA buffer to be corrupted. If all receive buffers are allocated on cache-line boundaries (both start and end of each buffer), you can invalidate the cache "early" under the assumption that there will be no read accesses to the read buffers themselves until after DMA is complete. IMHO it is better, even in this case., to invalidate cache after dma is done but before referencing the read data.

Quoted 33 lines · J. William Campbell

Quoted 22 lines · Albert ARIBAUD

Le 19/07/2011 10:43, Aneesh V a ?crit :

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

You would have to flush (before sending packets / starting external memory-to-device DMA) and invalidate (before reading received packets / after external device-to-memory DMA is done); using MMU and mapping cached/non-cached areas is IMO overkill, and will hurt CPU accesses to the xmit/receive buffers and descriptors.

So, you say actually what I did while exploring the problem would have been a
correct way of solving this problem?

Quoted 5 lines

Quoted 3 lines

Like this:

587 flush_cache(&fec->tbd_base[fec->tbd_index], 4);

This is what is needed assuming the below is initiating a memory to peripheral DMA. Is your buffer only 4 bytes long?

Generally:

- for sending data through a device that has its own, external, DMA engine, you'll obviously need to flush the data buffer(s) but also any DMA descriptors used by the engine, before you start the engine;

Quoted 5 lines

- for rceiving, if you have to set up receive descriptors, you must flush that before telling the device to enter receive mode (so that the device reads the descriptors as you wrote them), and you should invalidate the receive buffers at the latest when the device signals that data has been received,

Hi All,

Quoted 3 lines

or preferably long before (at the same time
you flushed the read descriptor, so that cache-related actions are grouped in the same place in the code).

Quoted 14 lines

I think this last statement is incorrect. You should invalidate the cache for the receive buffers just before you intend to reference them. If you do it right after receive mode is entered, subsequent access to items NEAR the receive buffer may reload the cache with receive buffer data before the dma is done, re-creating the problem you are trying to avoid. Also, I don't know if ARM cache is write-back or write -thru, but if it is write-back, the only way to avoid problems is to allocate the receive buffers on cache line boundaries, so no "nearby" writes can cause something in the DMA buffer to be corrupted. If all receive buffers are allocated on cache-line boundaries (both start and end of each buffer), you can invalidate the cache "early" under the assumption that there will be no read accesses to the read buffers themselves until after DMA is complete. IMHO it is better, even in this case., to invalidate cache after dma is done but before referencing the read data.

This is a critical observation, and one I was going to make if I had made it to the end of the thread and no one had already pointed it out. In fact, there is no way with the current implementation (that I can see) of the v7_dcache_inval_range function to correctly implement a cache enabled DMA driven driver without aligning buffers to cache line sizes. Below is the commit message from a recent patch I made locally to fix the Tegra MMC driver. I wanted to start a discussion on the list about forcing all buffers to be aligned to cache line sizes. The problem is that many buffers are stack allocated or part of structs that are of unknown alignment.

mmc: Tegra2: Enable dcache support by bouncing unaligned requests.

Dcache support was disabled due to dcache/dma interactions with unaligned buffers. When an unaligned buffer is used for DMA the first and last few bytes of the buffer will be clobbered by the dcache invalidate call that is required to make the contents of the buffer visible to the CPU post DMA. The reason that these bytes are clobbered is that the dcache invalidate code (which is the same as the linux kernels) checks for unaligned invalidates and first flushes the cache lines that are being partially invalidated. This is required because the cache lines may be shared with other variables, and may be dirty.

This flush however
writes over the first and last few bytes of the unaligned buffer with whatever happened to be in the cache.

There are a number of possible solutions:

1) Modify the invalidate code to first read the partial cache line and then invalidate and then write back just the valid part of the line. This suffers from a race condition with concurrent code in interrupt handlers or other CPUs.

2) Modify all of U-Boot to allocate buffers from a block allocation mechanism that ensures they are alligned on cache line boundaries. While this is possible, there are some cases where the public API passes down a buffer pointer all the way to the block read interface. So all stand alone U-Boot programs would have to be fixed as well.

3) Use a bounce buffer that is known to be alligned. Allocate the buffer in the Tegra MMC code and ensure it is large enough for the read/write as well as aligned correctly. This solution requires an extra memcpy of the data read or written and has a high water mark on memory consumption. It can be conditionally used only when the buffer passed in is unaligned.

This patch implements the third solution.

-Anton

If this is true, then it means that the cache is of type write-back (as opposed to write-thru). From a (very brief) look at the arm7 manuals, it appears that both types of cache may be present in the cpu. Do you know how this operates?

Le 20/07/2011 16:01, J. William Campbell a ?crit :

Quoted 10 lines · Albert ARIBAUD

Quoted 9 lines

Le 19/07/2011 22:11, J. William Campbell a ?crit :

Quoted 7 lines

If this is true, then it means that the cache is of type write-back (as
opposed to write-thru). From a (very brief) look at the arm7 manuals, it
appears that both types of cache may be present in the cpu. Do you know
how this operates?

Quoted 7 lines

Quoted 2 lines

Usually, copyback (rather than writeback) and writethough are modes of operation, not cache types.

Hi Albert,
One some CPUs both cache modes are available. On many other CPUs (I would guess most), you have one fixed mode available, but not both. I have always seen the two modes described as write-back and write-through, but I am sure we are talking about the same things.

We are. Copy-back is another name for write-back, not used by ARM but by some others.

Quoted 5 lines

The
examples that have both modes that I am familiar with have the mode as a "global" setting. It is not controlled by bits in the TLB or anything like that. How does it work on ARM? Is it fixed, globally, globally controlled, or controlled by memory management?

Well, it's a bit complicated, because it depends on the architecture version *and* implementation -- ARM themselves do not mandate things, and it is up to the SoC designer to specify what cache they want and what mode it supports, both at L1 and L2, in their specific instance of ARM cores. And yes, you can have memory areas that are write-back and others that are write-through in the same system.

Quoted 6 lines

If it is controlled by memory management, it looks to me like lots of problems could be avoided by operating with input type buffers set as write-through. One probably isn't going to be writing to input buffers much under program control anyway, so the performance loss should be minimal. This gets rid of the alignment restrictions on these buffers but not the invalidate/flush requirements.

There's not much you can do about alignment issues except align to cache line boundaries.

Quoted 4 lines

However, if memory management
is required to set the cache mode, it might be best to operate with the buffers and descriptors un-cached. That gets rid of the flush/invalidate requirement at the expense of slowing down copying from read buffers.

That makes 'best' a subjective choice, doesn't it? :)

Quoted 2 lines

Probably a reasonable price to pay for the associated simplicity.

Others would say that spending some time setting up alignments and flushes and invalidates is a reasonable price to pay for increased performance... That's an open debate where no solution is The Right One(tm).

For instance, consider the TFTP image reading. People would like the image to end up in cached memory because we'll do some checksumming on it before we give it control, and having it cached makes this step quite faster; but we'll lose that if we put it in non-cached memory because it comes through the Ethernet controller's DMA; and it would be worse to receive packets in non-cached memory only to move their contents into cached memory later on.

I think properly aligning descriptors and buffers is enough to avoid the mixed flush/invalidate line issue, and wisely putting instruction barriers should be enough to get the added performance of cache without too much of the hassle of memory management.

Quoted 3 lines

Best Regards,
Bill Campbell

Amicalement,

Quoted 45 lines · Albert ARIBAUD

Le 20/07/2011 16:01, J. William Campbell a ?crit :

Quoted 10 lines · Albert ARIBAUD

Quoted 9 lines

Le 19/07/2011 22:11, J. William Campbell a ?crit :

Quoted 7 lines

If this is true, then it means that the cache is of type write-back (as
opposed to write-thru). From a (very brief) look at the arm7 manuals, it
appears that both types of cache may be present in the cpu. Do you know
how this operates?

Quoted 7 lines

Quoted 2 lines

Usually, copyback (rather than writeback) and writethough are modes of operation, not cache types.

Hi Albert,
One some CPUs both cache modes are available. On many other CPUs (I would guess most), you have one fixed mode available, but not both. I have always seen the two modes described as write-back and write-through, but I am sure we are talking about the same things.

We are. Copy-back is another name for write-back, not used by ARM but by some others.

Quoted 5 lines

The
examples that have both modes that I am familiar with have the mode as a "global" setting. It is not controlled by bits in the TLB or anything like that. How does it work on ARM? Is it fixed, globally, globally controlled, or controlled by memory management?

Well, it's a bit complicated, because it depends on the architecture version *and* implementation -- ARM themselves do not mandate things, and it is up to the SoC designer to specify what cache they want and what mode it supports, both at L1 and L2, in their specific instance of ARM cores. And yes, you can have memory areas that are write-back and others that are write-through in the same system.

Quoted 6 lines

If it is controlled by memory management, it looks to me like lots of problems could be avoided by operating with input type buffers set as write-through. One probably isn't going to be writing to input buffers much under program control anyway, so the performance loss should be minimal. This gets rid of the alignment restrictions on these buffers but not the invalidate/flush requirements.

There's not much you can do about alignment issues except align to cache line boundaries.

Quoted 6 lines

Quoted 4 lines

However, if memory management
is required to set the cache mode, it might be best to operate with the buffers and descriptors un-cached. That gets rid of the flush/invalidate requirement at the expense of slowing down copying from read buffers.

That makes 'best' a subjective choice, doesn't it? :)

Hi All,
Yes,it probably depends on the usage.

Quoted 16 lines

Quoted 2 lines

Probably a reasonable price to pay for the associated simplicity.

Others would say that spending some time setting up alignments and flushes and invalidates is a reasonable price to pay for increased performance... That's an open debate where no solution is The Right One(tm).

For instance, consider the TFTP image reading. People would like the image to end up in cached memory because we'll do some checksumming on it before we give it control, and having it cached makes this step quite faster; but we'll lose that if we put it in non-cached memory because it comes through the Ethernet controller's DMA; and it would be worse to receive packets in non-cached memory only to move their contents into cached memory later on.

Quoted 4 lines

I think properly aligning descriptors and buffers is enough to avoid the mixed flush/invalidate line issue, and wisely putting instruction barriers should be enough to get the added performance of cache without too much of the hassle of memory management.

I am pretty sure that all the drivers read the input data into intermediate buffers in all cases. There is no practical way to be sure the next packet received is the "right one" for the tftp. Plus there are headers involved, and furthermore there is no way to ensure that a tftp destination is located on a sector boundary. In short, you are going to copy from an input buffer to a destination.
However, it is still correct that copying from an non-cached area is slower than from cached areas, because of burst reads vs. individual reads. However, I doubt that the u-boot user can tell the difference, as the network latency will far exceed the difference in copy time. The question is, which is easier to do, and that is probably a matter of opinion. However, it is safe to say that so far a cached solution has eluded us. That may be changing, but it would still be nice to know how to allocate a section of un-cached RAM in the ARM processor, in so far as the question has a single answer! That would allow easy portability of drivers that do not know about caches, of which there seems to be many.

Quoted 4 lines

However, it is still correct that copying from an non-cached area is slower than from cached areas, because of burst reads vs. individual reads. However, I doubt that the u-boot user can tell the difference, as the network latency will far exceed the difference in copy time.

Quoted 7 lines

The
question is, which is easier to do, and that is probably a matter of opinion. However, it is safe to say that so far a cached solution has eluded us. That may be changing, but it would still be nice to know how to allocate a section of un-cached RAM in the ARM processor, in so far as the question has a single answer! That would allow easy portability of drivers that do not know about caches, of which there seems to be many.

I agree. Unfortunately, my time is up for now, and I can't go on with trying to fix this driver. Maybe I'll pick up after my vacation.
As for now I settled for the ugly solution of keeping dcache disabled while ethernet is being used :-(

IMHO, doing cache maintenance all over the driver is not an easy or nice solution. Implementing a non-cached memory pool in the MMU and a corresponding dma_malloc() sounds like much more universally applicable to any driver.

Best regards,

Le 21/07/2011 08:48, David Jander a ?crit :

Quoted 5 lines

Quoted 5 lines

However, it is still correct that copying from an non-cached area is slower than from cached areas, because of burst reads vs. individual reads. However, I doubt that the u-boot user can tell the difference, as
the network latency will far exceed the difference in copy time.

That's assuming cache is only for networking. There can be DMA engines in a lot of other peripherals which do not have the same latency as network (and then, even for networking, TFTP can be done from a very nearby server, possibly even on the same Ethernet segment).

Quoted 9 lines

Quoted 9 lines

The
question is, which is easier to do, and that is probably a matter of opinion. However, it is safe to say that so far a cached solution has eluded us. That may be changing, but it would still be nice to know how to allocate a section of un-cached RAM in the ARM processor, in so far as the question has a single answer! That would allow easy portability of drivers that do not know about caches, of which there seems to be many.

That is one approach, which I think prevents cache from being used beyond caching pure CPU-used DRAM.

Quoted 7 lines

I agree. Unfortunately, my time is up for now, and I can't go on with trying
to fix this driver. Maybe I'll pick up after my vacation. As for now I settled for the ugly solution of keeping dcache disabled while
ethernet is being used :-(

Make sure you flush before disabling. :)

Quoted 4 lines

IMHO, doing cache maintenance all over the driver is not an easy or nice solution. Implementing a non-cached memory pool in the MMU and a corresponding
dma_malloc() sounds like much more universally applicable to any driver.

I think cache maintenance is feasible if one makes sure the cached areas used by the driver are properly aligned, which simplifies things a lot: you don't have to care for flush-invalidate or just-in-time invalidate, you just have to flush before sending and invalidate before reading.

Quoted 2 lines

Best regards,

Amicalement,

There are a number of possible solutions:

1) Modify the invalidate code to first read the partial cache line and then invalidate and then write back just the valid part of the line. This suffers from a race condition with concurrent code in interrupt handlers or other CPUs.