Uboot error: address not aligned in v7_dcache_inval_range

Hi,

I compiled the current u-boot from
https://github.com/jwrdegoede/u-boot-sunxi.git for cubieboard2, and wrote it to the SD card. I was trying to boot the kernel on my computer over network by tftp, however it failed when I ran "dhcp" or "tftp" command in uboot with a tons of:

ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x7fb67820

struct dw_eth_dev contains fields which are accessed via DMA, so make sure it is aligned to a dma boundary. Without this I see:

    ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0

Signed-off-by Ian Campbell <[email protected]>

---
 drivers/net/designware.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/designware.c +0 -0

index 6ece479..1120f70 100644

struct dw_eth_dev contains fields which are accessed via DMA, so make sure it is aligned to a dma boundary. Without this I see:
ERROR: v7_dcache_inval_range - start address is not aligned -
0x7fb677e0

Signed-off-by Ian Campbell <[email protected]>

---
 drivers/net/designware.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/designware.c +0 -0

index 6ece479..1120f70 100644
@@ -412,7 +412,8 @@ int designware_initialize(ulong base_addr, u32

interface) * Since the priv structure contains the descriptors which need a strict * buswidth alignment, memalign is used to allocate memory

Quoted 2 lines

Some platforms cannot invalidate the cache at four byte intervals, so invalidate the entire descriptor.

Wouldn't it be simpler to make invalidate_dcache_range round up to the nearest aligned boundaries?

Can there ever be situations where invalidating "too much" could be a problem?

drivers/net/designware.c +0 -4

-	/* Invalidate only "status" field for the following check */
-	invalidate_dcache_range((unsigned long)&desc_p->txrx_status,
-				(unsigned long)&desc_p->txrx_status +
-				sizeof(desc_p->txrx_status));

1. Don't invalidate "sizeof(struct dmamacdescr)" but only "roundup(sizeof(desc_p->txrx_status), ARCH_DMA_MINALIGN))".

2. In the following lines implements rounding as well:

3. Check carefully if there're other instances of probably unaligned cache operations.

4. Why don't you squeeze all 3 patches in 1 and name it like "fix alignment issues with caches on some platforms"? Basically with all 3 patches you fix one and only issue and application of any one of those 3 patches doesn't solve your problem, right?

Quoted 2 lines · Alexey Brodkin

1. Don't invalidate "sizeof(struct dmamacdescr)" but only "roundup(sizeof(desc_p->txrx_status), ARCH_DMA_MINALIGN))".

OK. (Although given the realities of the real world values of ARCH_DMA_MINALIGN on every arch and the sizes of the structs & fields involved this isn't actually buying you anything at all)

Quoted 1 line

2. In the following lines implements rounding as well:

Will fix as well.

Quoted 2 lines

3. Check carefully if there're other instances of probably unaligned cache operations.

I'm not seeing any others, in practice or by eye-balling the code.

Quoted 4 lines

4. Why don't you squeeze all 3 patches in 1 and name it like "fix alignment issues with caches on some platforms"? Basically with all 3 patches you fix one and only issue and application of any one of those 3 patches doesn't solve your problem, right?

These are the issues as I discovered them one by one. I can fold them if you like but doing them separately will aid bisection if one of them turns out to be wrong in some way. As you prefer.

I thought a bit more about this situation and now I'm not that sure if we need to align addresses we pass to cache invalidate/flush functions.

Because IMHO drivers shouldn't care about specifics of particular platform or architecture. Otherwise we'll need to patch each and every driver only for cache invalidate/flush functions. I looked how these functions are used in other drivers and see that in most of cases no additional alignment precautions were implemented. People just pass start and end addresses.

In its turn platform and architecture provides cache invalidate/flush functions implement its functionality depending on hardware specifics.

For example on architectures that may only flush/invalidate with granularity of 1 cache line cache invalidate/flush functions make sure to start processing from the start of the cache line to which start address falls and end processing when cache line where end address falls is processed.

I may assume that there're architectures that automatically understand from which cache line to start and at which line to stop processing.

But if your architecture requires cache line aligned addresses to be
used for start/end addresses you may look for examples in ARC
(http://git.denx.de/?p=u-boot/u-boot-arc.git;a=blob;f=arch/arc/cpu/arc700/cache.c),, MIPS (http://git.denx.de/?p=u-boot/u-boot-arc.git;a=blob;f=arch/mips/cpu/mips32/cpu.c), SH (http://git.denx.de/?p=u-boot/u-boot-arc.git;a=blob;f=arch/sh/cpu/sh4/cache.c),

and what's interesting even implementation you use have semi-proper start/end addresses handling -
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/lib/cache-pl310.c

Here's your invalidation procedure:
============
/* invalidate memory from start to stop-1 */ void v7_outer_cache_inval_range(u32 start, u32 stop)

{
	/* PL310 currently supports only 32 bytes cache line */
	u32 pa, line_size = 32;

	/*
	 * If start address is not aligned to cache-line do not
	 * invalidate the first cache-line
	 */
	if (start & (line_size - 1)) {
		printf("ERROR: %s - start address is not aligned - 0x%08x\n",
			__func__, start);
		/* move to next cache line */
		start = (start + line_size - 1) & ~(line_size - 1);
	}

	/*
	 * If stop address is not aligned to cache-line do not
	 * invalidate the last cache-line
	 */
	if (stop & (line_size - 1)) {
		printf("ERROR: %s - stop address is not aligned - 0x%08x\n",
			__func__, stop);
		/* align to the beginning of this cache line */
		stop &= ~(line_size - 1);
	}

	for (pa = start; pa < stop; pa = pa + line_size)
		writel(pa, &pl310->pl310_inv_line_pa);

	pl310_cache_sync();
}

============

1. I don't understand why start from the next cache line if start address is not aligned to cache line boundary? I'd say that you want to invalidate cache line that contains unaligned start address. Otherwise first bytes won't be invalidated, right?

2. Why do we throw _error_ message. I may understand if you emit _warning_ message in case of debug build (with DEBUG defined). Well in current implementation (see 1) it could be error because behavior is really dangerous. But if you start from correct cache line only warning in debug mode makes sense (IMHO).

3. Stop/end address in contrast might need to be extended depending on HW implementation (see above comment).

And here's your flush procedure:
===========
void v7_outer_cache_flush_range(u32 start, u32 stop)

{
	/* PL310 currently supports only 32 bytes cache line */
	u32 pa, line_size = 32;

	/*
	 * Align to the beginning of cache-line - this ensures that
	 * the first 5 bits are 0 as required by PL310 TRM
	 */
	start &= ~(line_size - 1);

	for (pa = start; pa < stop; pa = pa + line_size)
		writel(pa, &pl310->pl310_clean_inv_line_pa);

	pl310_cache_sync();
}

===========

Which looks very correct to me. I'm wondering if there was a reason to have so different implementation of functions that do very similar things.

So at this point I would ask you to modify cache invalidate function for your architecture. This way you prevent mentioned issues with other drivers.

Quoted 10 lines

I'm not seeing any others, in practice or by eye-balling the code.

Quoted 4 lines

4. Why don't you squeeze all 3 patches in 1 and name it like "fix alignment issues with caches on some platforms"? Basically with all 3 patches you fix one and only issue and application of any one of those 3 patches doesn't solve your problem, right?

These are the issues as I discovered them one by one. I can fold them if you like but doing them separately will aid bisection if one of them turns out to be wrong in some way. As you prefer.

Keeping in mind things written above I'd say that patches 2 & 3 are not needed at all, while patch 1 makes perfect sense and fixes an obvious issue.

Regards,
Alexey

This is the driver for one particular ARM cache controller and not the one used for the SoC. In any case it does "proper" start/end handling only for cache flush operations, not cache invalidate.

Cache invalidate is a potentially destructive operation (throwing away data in the caches), having it operate on anything more than the precise region requested would be very surprising to almost anyone I think.

I think you are missing the important differences between a cache flush and a cache invalidate.

And in this situation IMHO the only safe solution could be in proper design of data layout. In other words we need to keep independent data blocks aligned to cache line.

And as you may see from "designware.h" buffer descriptor structure is aligned:

==============

struct dmamacdescr {
	u32 txrx_status;
	u32 dmamac_cntl;
	void *dmamac_addr;
	struct dmamacdescr *dmamac_next;
} __aligned(ARCH_DMA_MINALIGN);

==============

Regards,
Alexey

1. Don't invalidate "sizeof(struct dmamacdescr)" but only "roundup(sizeof(desc_p->txrx_status), ARCH_DMA_MINALIGN))".

Signed-off-by Ian Campbell <[email protected]>

---
 drivers/net/designware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/designware.c +2 -1

index 7d14cec..30446d3 100644
@@ -354,7 +354,7 @@ static int dw_eth_recv(struct eth_device *dev)
                /* Invalidate received data */
                invalidate_dcache_range((unsigned long)desc_p->dmamac_addr,
                                        (unsigned long)desc_p->dmamac_addr
+
-                                       length);
+                                       roundup(length,
 ARCH_DMA_MINALIGN));
 
                NetReceive(desc_p->dmamac_addr, length);

?With these three patches, I don't see ?

--
1.9.0

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 11 lines · Shixin Zeng

Hi,

I compiled the current u-boot from
https://github.com/jwrdegoede/u-boot-sunxi.git for cubieboard2, and wrote it to the SD card. I was trying to boot the kernel on my computer over network by tftp, however it failed when I ran "dhcp" or "tftp" command in uboot with a tons of:

ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x7fb67820

I'm seeing this on Cubieboard2 and Cubietruck. It appears to be down to a change to the upstream designware driver:

commit 50b0df814b0f75c08a3d45a017016a75af3edb5d

Author: Alexey Brodkin <[email protected]>
Date:   Wed Jan 22 20:49:09 2014 +0400

net/designware: make driver compatible with data cache

Up until now this driver only worked with data cache disabled. To make it work with enabled data cache following changes were
required:

* Flush Tx/Rx buffer descriptors their modification
* Invalidate Tx/Rx buffer descriptors before reading its values
* Flush cache for data passed from CPU to GMAC
* Invalidate cache for data passed from GMAC to CPU

http://git.denx.de/?p=u-boot.git;a=commit;h=50b0df814b0f75c08a3d45a017016a75af3edb5d

I suppose this was only tested on some architecture which allows DMA flush/invaidation at a fairly fine granularity (at least down to 4 byte boundaries)

Making sure that struct dw_eth_dev is DMA aligned helps with the invalidate of the descriptors in dw_eth_recv (see below) but with that the invalidate of the txrx_status field in dw_eth_send is still problematic -- the field is only 4 bytes, so although the descriptor is aligned the end is not.

?

Ian.

commit 8878d858ede12584b885fa9439f9093bf2186a90

Author: Ian Campbell <[email protected]>
Date:   Sat Apr 19 14:16:04 2014 +0100

net/designware: ensure device private data is DMA aligned.

struct dw_eth_dev contains fields which are accessed via DMA, so make
sure it
is aligned to a dma boundary. Without this I see:
ERROR: v7_dcache_inval_range - start address is not aligned -
0x7fb677e0

Signed-off-by Ian Campbell <[email protected]>

drivers/net/designware.c +0 -0

index 6ece479..1120f70 100644
@@ -412,7 +412,8 @@ int designware_initialize(ulong base_addr, u32
 interface)
         * Since the priv structure contains the descriptors which need a

strict
* buswidth alignment, memalign is used to allocate memory */
- priv = (struct dw_eth_dev *) memalign(16, sizeof(struct
dw_eth_dev));

drivers/net/designware.c +2 -1

+       priv = (struct dw_eth_dev *) memalign(ARCH_DMA_MINALIGN,
+                                             sizeof(struct dw_eth_dev));
        if (!priv) {
                free(dev);
                return -ENOMEM;
 
 
--

You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 11 lines · Shixin Zeng

Hi,

I compiled the current u-boot from
https://github.com/jwrdegoede/u-boot-sunxi.git for cubieboard2, and wrote it to the SD card. I was trying to boot the kernel on my computer over network by tftp, however it failed when I ran "dhcp" or "tftp" command in uboot with a tons of:

ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x7fb67820

I'm seeing this on Cubieboard2 and Cubietruck. It appears to be down to a change to the upstream designware driver:

commit 50b0df814b0f75c08a3d45a017016a75af3edb5d

Author: Alexey Brodkin <[email protected]>
Date:   Wed Jan 22 20:49:09 2014 +0400

net/designware: make driver compatible with data cache

Up until now this driver only worked with data cache disabled. To make it work with enabled data cache following changes were
required:

* Flush Tx/Rx buffer descriptors their modification
* Invalidate Tx/Rx buffer descriptors before reading its values
* Flush cache for data passed from CPU to GMAC
* Invalidate cache for data passed from GMAC to CPU

http://git.denx.de/?p=u-boot.git;a=commit;h=50b0df814b0f75c08a3d45a017016a7
5af3edb5d

I suppose this was only tested on some architecture which allows DMA flush/invaidation at a fairly fine granularity (at least down to 4 byte boundaries)

This was a sheer luck this ever worked. Looking at the entire driver, to fix all your issues with DMA and caches, it would be sufficient to re-align "struct dw_eth_dev" properly.

See drivers/net/designware.h:
1) struct dmamacdescr {} is already __aligned(ARCH_DMA_MINALIGN) => This structure, if aligned in memory to proper boundary, can be flushed/ invalidated without problems.
2) struct dw_eth_dev {} can be aligned to ANY 4-byte boundary
But this structure contains two arrays of struct dmamacdescr {} , which each have their elements' lenght aligned to ARCH_DMA_MINALIGN

Solution:

Your patch [1/3] and reorder the structure in designware.h so that the struct dmamacdescr tx_mac_descrtable[]
struct dmamacdescr rx_mac_descrtable[]
are first and anything that does not need to be aligned follows. This way, the DMA descriptors will always be aligned and you need not worry about the flushes. You don't even need to ROUNDUP their length, since they are already fine.

When reordering the struct dw_eth_dev {}, make sure to add a comment about the alignment.

Quoted 19 lines · Marek Vasut

This was a sheer luck this ever worked. Looking at the entire driver, to fix all your issues with DMA and caches, it would be sufficient to re-align "struct dw_eth_dev" properly.

See drivers/net/designware.h:
1) struct dmamacdescr {} is already __aligned(ARCH_DMA_MINALIGN)

=> This structure, if aligned in memory to proper boundary, can be flushed/

invalidated without problems.

2) struct dw_eth_dev {} can be aligned to ANY 4-byte boundary

But this structure contains two arrays of struct dmamacdescr {} , which each have their elements' lenght aligned to ARCH_DMA_MINALIGN

Solution:

Quoted 7 lines

Your patch [1/3] and reorder the structure in designware.h so that the struct dmamacdescr tx_mac_descrtable[]
struct dmamacdescr rx_mac_descrtable[]
are first and anything that does not need to be aligned follows. This way, the DMA descriptors will always be aligned and you need not worry about the flushes. You don't even need to ROUNDUP their length, since they are already fine.

That sounds like a good plan. I'll take a look.

Quoted 2 lines

When reordering the struct dw_eth_dev {}, make sure to add a comment about the alignment.

Of course.

Quoted 31 lines · Ian Campbell

Quoted 11 lines · Shixin Zeng

Hi,

I compiled the current u-boot from
https://github.com/jwrdegoede/u-boot-sunxi.git for cubieboard2, and wrote it to the SD card. I was trying to boot the kernel on my computer over network by tftp, however it failed when I ran "dhcp" or "tftp" command in uboot with a tons of:

ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x7fb67820

I'm seeing this on Cubieboard2 and Cubietruck. It appears to be down to a change to the upstream designware driver:

commit 50b0df814b0f75c08a3d45a017016a75af3edb5d

Author: Alexey Brodkin <[email protected]>
Date:   Wed Jan 22 20:49:09 2014 +0400

net/designware: make driver compatible with data cache

Up until now this driver only worked with data cache disabled. To make it work with enabled data cache following changes were
required:

* Flush Tx/Rx buffer descriptors their modification
* Invalidate Tx/Rx buffer descriptors before reading its values
* Flush cache for data passed from CPU to GMAC
* Invalidate cache for data passed from GMAC to CPU

Quoted 6 lines

http://git.denx.de/?p=u-boot.git;a=commit;h=50b0df814b0f75c08a3d45a017016a7
5af3edb5d

I suppose this was only tested on some architecture which allows DMA flush/invaidation at a fairly fine granularity (at least down to 4 byte boundaries)

This was a sheer luck this ever worked. Looking at the entire driver, to fix all your issues with DMA and caches, it would be sufficient to re-align "struct dw_eth_dev" properly.

See drivers/net/designware.h:
1) struct dmamacdescr {} is already __aligned(ARCH_DMA_MINALIGN) => This structure, if aligned in memory to proper boundary, can be flushed/ invalidated without problems.
2) struct dw_eth_dev {} can be aligned to ANY 4-byte boundary
But this structure contains two arrays of struct dmamacdescr {} , which each have their elements' lenght aligned to ARCH_DMA_MINALIGN

Solution:

Your patch [1/3] and reorder the structure in designware.h so that the struct dmamacdescr tx_mac_descrtable[]
struct dmamacdescr rx_mac_descrtable[]
are first and anything that does not need to be aligned follows. This way, the DMA descriptors will always be aligned and you need not worry about the flushes. You don't even need to ROUNDUP their length, since they are already fine.

Quoted 59 lines · Marek Vasut

Quoted 32 lines · Ian Campbell

Quoted 9 lines · Shixin Zeng

Hi,

I compiled the current u-boot from
https://github.com/jwrdegoede/u-boot-sunxi.git for cubieboard2, and wrote it to the SD card. I was trying to boot the kernel on my computer over network by tftp, however it failed when I ran "dhcp" or "tftp" command in uboot with a tons of:

Quoted 3 lines

ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x7fb67820

I'm seeing this on Cubieboard2 and Cubietruck. It appears to be down to a change to the upstream designware driver:

commit 50b0df814b0f75c08a3d45a017016a75af3edb5d

Author: Alexey Brodkin <[email protected]>
Date:   Wed Jan 22 20:49:09 2014 +0400

net/designware: make driver compatible with data cache

Up until now this driver only worked with data cache disabled. To make it work with enabled data cache following changes were

required:
* Flush Tx/Rx buffer descriptors their modification
* Invalidate Tx/Rx buffer descriptors before reading its values
* Flush cache for data passed from CPU to GMAC
* Invalidate cache for data passed from GMAC to CPU

Quoted 6 lines

http://git.denx.de/?p=u-boot.git;a=commit;h=50b0df814b0f75c08a3d45a0170
16a7 5af3edb5d

I suppose this was only tested on some architecture which allows DMA flush/invaidation at a fairly fine granularity (at least down to 4 byte boundaries)

This was a sheer luck this ever worked. Looking at the entire driver, to fix all your issues with DMA and caches, it would be sufficient to re-align "struct dw_eth_dev" properly.

See drivers/net/designware.h:
1) struct dmamacdescr {} is already __aligned(ARCH_DMA_MINALIGN)

=> This structure, if aligned in memory to proper boundary, can be flushed/

invalidated without problems.

2) struct dw_eth_dev {} can be aligned to ANY 4-byte boundary

But this structure contains two arrays of struct dmamacdescr {} , which each have their elements' lenght aligned to ARCH_DMA_MINALIGN

Solution:

Quoted 7 lines

Your patch [1/3] and reorder the structure in designware.h so that the struct dmamacdescr tx_mac_descrtable[]
struct dmamacdescr rx_mac_descrtable[]
are first and anything that does not need to be aligned follows. This way, the DMA descriptors will always be aligned and you need not worry about the flushes. You don't even need to ROUNDUP their length, since they are already fine.

Unfortunately this isn't sufficient, at least a change in the spirit of my second patch (to flush the entire descriptor) is also needed, because flushing just a subfield misaligns things again.

And my patch 3/3 is still needed because it deals with the data itself and not the descriptors.

So having done all that it doesn't seem that reordering dw_eth_dev is necessary.

Quoted 2 lines · Ian Campbell

So having done all that it doesn't seem that reordering dw_eth_dev is necessary.

Reordering dw_eth_dev is necessary. Look: