[U-Boot] [PATCH] nios2: convert cache flush to use dm cpu data

[Marek Vasut]

On Sunday, October 11, 2015 at 02:38:35 AM, Thomas Chou wrote:
> Hi Marek,

Hi,

> On 10/11/2015 02:18 AM, Marek Vasut wrote:
> > Then you'd also need means to allocate variables to aligned memory
> > location to prevent invalid cache flush. (Linux does this with it's DMA
> > API). We are much simpler and thus this abstraction is still not
> > available. I wonder if the overhead of DMA API would be high or not for
> > U-Boot.
> 
> I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to
> allocate DMA buffers so that they are cache aligned.

That and include/memalign.h , which contains macros that are used to align
variables.

> > It is even worse if the cache flush operators permit incorrect cache
> > flushes or invalidations. Like I mentioned before, this can lead to hard
> > to debug problems when using DMA (at least on ARM).
> 
> I would suggest debug check should be left as for debug only. The
> definition of common functions should be kept as it is more important
> than coding style.

Uh yes, that's what arm926 cache functions do, they're debug only.

> I debugged DMA issues a lot in the past until I realized the importance
> of aligned buffers. So there should be a developer's guideline.

For what exactly?

> But it is even much more difficult when something you believed does not
> work as expected, what is taken as common sense. It will trap a lot of
> developers when they called your flush cache functions but was skipped
> just because, eg, the end of packets are not aligned which is usually
> the case.

This is good, it should bite them, because this is a bug. If, on the other
hand, you will paper over such bugs by adding crap to the cache ops, there
will be even worse bugs coming for you, like variables which are sitting in
the same cacheline as your unaligned buffer that you want to invalidate or
flush will possibly get trashed by such cache operation.

Consider this:

cacheline 0: [ variable A ; buffer B ......... ]
cacheline 1: [ buffer B ......... ; Empty .... ]

Now you do the following:

1) Variable A = 0;
2) Flush buffer B (which is unaligned, so flush cacheline 0 and 1)
3) Start DMA to buffer B
4) Variable A = 1;
5) Check if DMA finished, it did
6) Invalidate buffer B ... oh, but it's unaligned, let's invalidate
   everything around it, which is cacheline 0 and 1.
7) What is the value of variable A ? Oh, it's fetched from memory and
   it's 0 there, even though we did set it to 1 ...

> I would suggest that, with the best of my knowledge, please change the
> range check to a debug probe, and restore the cache flush functions to
> the common definition.

See above, does my example make it clear why we should never ever hide
bugs in the cache ops code ?

Best regards,
Marek Vasut