[U-Boot] [PATCH 2/9] CACHE: Add cache_aligned() macro

[Marek Vasut]

Dear Scott Wood,

> On 06/24/2012 07:17 PM, Marek Vasut wrote:
> > This macro returns 1 if the argument (address) is aligned, returns
> > zero otherwise. This will be used to test user-supplied address to
> > various commands to prevent user from loading data to/from unaligned
> > address when using caches.
> > 
> > This is made as a macro, because macros are expanded where they are
> > used. Therefore it can be easily instrumented to report position of
> > the fault.
> > 
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Cc: Wolfgang Denk <wd at denx.de>
> > ---
> > 
> >  include/common.h |   18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/include/common.h b/include/common.h
> > index 322569e..17c64b0 100644
> > --- a/include/common.h
> > +++ b/include/common.h
> > @@ -730,6 +730,24 @@ void	invalidate_dcache_range(unsigned long start,
> > unsigned long stop);
> > 
> >  void	invalidate_dcache_all(void);
> >  void	invalidate_icache_all(void);
> > 
> > +/* Test if address is cache-aligned. Returns 0 if it is, 1 otherwise. */
> > +#define cacheline_aligned(addr)						
\
> > +	({								\
> > +	int __ret;							\
> > +	if (!dcache_status()) {						\
> > +		__ret = 1;						\
> > +	} else if ((addr) & (ARCH_DMA_MINALIGN - 1)) {			\
> > +		puts("Align load address to "				\
> > +			__stringify(ARCH_DMA_MINALIGN)			\
> > +			" bytes when using caches!\n");			\
> > +		__ret = 0;						\
> > +	} else {							\
> > +		__ret = 1;						\
> > +	}								\
> > +	__ret;								\
> > +	})
> 
> What if it's a store rather than a load?

Goot point #1

> If this is only supposed to be
> used for loads (because on a store you can flush the cache rather than
> invalidate), it's not labelled that way, the changelog says "to/from
> unaligned address", and the caller might be common code that doesn't
> know which direction the transfer is in.

And we're back at the flush/invalidate thing. This is what I'd really love to 
understand once and for all:

1) We must prevent user from loading to address 0x.......1 (unaligned), I'll get 
back to this later on.

2) If the user had address that starts at aligned location but ends at 
unaligned:

Terminology:
va -- unrelated variable
[****] -- aligned range

2a) LOAD from external source to local address:
The cache must be invalidated over that area. I see the following issue:

[---------buffer---------][va]
[********][********][********]

So consider the scenario where
i) va is written in u-boot
ii) data arrive into the buffer via DMA at the same time
iii) cache is invalidated over whole buffer

This will cause variable "va" to be lost forever. The "va" variable might as 
well be user data, therefore in order to protect those, we should first flush 
the user-supplied area.

2b) STORE from local address to external source:
Basically the inverted problem as in 2a), but if the driver is written properly, 
shouldn't be any issue.

> Besides, it would be awkward
> user interface to allow an address to be used in one direction but not
> the other.

Correct, it should be possible to adjust the message.

> What if the caller wants to try a different strategy if this returns 0,
> rather than print an error?

Good point.

> Why should the success of a command depend on whether caches are
> enabled?

On less capable architectures, flushing caches over unaligned area causes real 
mayhem (and DMA depends on this) and it's really tough to debug. If the caches 
are off, it's all good.

Good point is, that this code should be enabled on per-CPU-model basis probably? 
Or entirely configurable in the include/configs/....

> If we're going to forbid unaligned addresses in certain
> contexts, shouldn't it always be forbidden to ensure consistent user
> experience?

This is a good argument ... I wonder, let's hear others opinions.

> Or if we're going to be picky about when we reject it, why
> don't we care whether the device in question does DMA, and whether that
> DMA is coherent?

Correct, good point. But then this is something that should be added into the 
upcomming uboot driver model!

> -Scott

Best regards,
Marek Vasut