On Friday 17 February 2012 05:50:21 Puneet Saxena wrote:
This aligns buffers,passed to usb controller at dcache boundary.
Buffer "stop" address = "start" address + size. Stop address can not be aligned even though "start" address is aligned as size does not fall at cache line boundary. Therefore "debug" in place of "printf" is added in "arch/arm/cpu/armv7/cache_v7.c". Cleaning and invalidating last cache line to avoid stale data to be reused.
the point is to align things based on the DMA constraints, not cacheline constraints (with the assumption that DMA constraints will always be equal or larger than cachelines and never smaller). so using CONFIG_SYS_CACHELINE_SIZE is wrong. this is exactly why we have ARCH_DMA_MINALIGN.
Ignoring "checkpatch.pl" warnings to replace "__attribute__((packed))" by "__packed" and "__attribute__((aligned(size)))" by "__aligned(size)". As "__GNUC__" directive should be defined to use "__packed" and "__aligned(size)" and current patch is not supporting it, ignoring the "checkpatch.pl" warning.
i don't understand this. we have linux/compiler.h which provides these macros so you should be able to use them.
so NAK to this implementation for the reasons above
--- a/arch/arm/cpu/armv7/cache_v7.c +++ b/arch/arm/cpu/armv7/cache_v7.c
/*
* We can't avoid stop address not aligned with cache-line.* Allocating cache -aligned buffer might waste memory.
- If stop address is not aligned to cache-line do not
* invalidate the last cache-line
* invalidate the last cache-line and flush the last* line to prevent affecting somebody else's buffer.
printf("ERROR: %s - stop address is not aligned - 0x%08x\n",
debug("ERROR: %s - stop address is not aligned - 0x%08x\n", __func__, stop);v7_dcache_clean_inval_range(stop, stop + 1, line_len);
pretty sure this is ignoring the problem and so shouldn't get merged
--- a/common/usb.c +++ b/common/usb.c
-static struct devrequest setup_packet; +#ifdef CONFIG_SYS_CACHELINE_SIZE
- static struct devrequest setup_packet
__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));+#else
- static struct devrequest setup_packet;
+#endif
this is used in only one function, so it'd be better to simply move it to that one function rather than keeping it as a global.
--- a/common/usb_storage.c +++ b/common/usb_storage.c
-static unsigned char usb_stor_buf[512]; +#ifdef CONFIG_SYS_CACHELINE_SIZE
- static unsigned char usb_stor_buf[512]
__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));+#else
- static unsigned char usb_stor_buf[512];
+#endif
Marek has already posted a patch fixing this
--- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -132,8 +132,20 @@ static void cache_qtd(struct qTD *qtd, int flush) int len = (qtd->qt_token & 0x7fff0000) >> 16;
flush_invalidate((u32)qtd, sizeof(struct qTD), flush);
- /*
* Invalidate cache for the aligned address* and for the data length which is DMAed.* Do not invalidate again intermidiate address* as it may not be aligned.*/+#if CONFIG_SYS_CACHELINE_SIZE
- if (!((u32) ptr & (CONFIG_SYS_CACHELINE_SIZE - 1)) && len)
flush_invalidate((u32)ptr, len, flush);+#else if (ptr && len) flush_invalidate((u32)ptr, len, flush); +#endif }
ugh no. existing code is correct.
--- a/include/scsi.h +++ b/include/scsi.h @@ -26,7 +26,13 @@
typedef struct SCSI_cmd_block{ unsigned char cmd[16]; /* command
*/
- unsigned char sense_buf[64]; /* for request sense */
- /* for request sense */
+#ifdef CONFIG_SYS_CACHELINE_SIZE
- unsigned char sense_buf[64]
__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));+#else
- unsigned char sense_buf[64];
+#endif
pretty sure this is wrong
--- a/include/usb.h +++ b/include/usb.h @@ -109,7 +109,13 @@ struct usb_device { int epmaxpacketout[16]; /* OUTput endpoint specific maximums */
int configno; /* selected config number */
- struct usb_device_descriptor descriptor; /* Device Descriptor */
/* Device Descriptor */+#ifdef CONFIG_SYS_CACHELINE_SIZE
- struct usb_device_descriptor descriptor
__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));+#else
- struct usb_device_descriptor descriptor;
+#endif struct usb_config config; /* config descriptor */
int have_langid; /* whether string_langid is valid yet */
as is this -mike