USB and cache related fixes
Subsystems (5)
cover USB and cache related fixes
Hey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32). First, the dcache_off call that ehci-omap had been doing was now not happening soon enough to paper over the cache issues. This call is removed in patch 1. Second, when we have dcache support compiled in but turned off via 'dcache off' the cache routines spam the console about alignment issues when a cache flush is attempted. This is a problem in that it makes operations extremely slow (as we're spending all our time spitting messages to console). The second patch makes the flush routines return when the dcache is off. The last two patches deal with the same problem, for EHCI and for MUSB. The USB spec says that 32 bytes is the minimum alignment but we need larger alignment when the cache is larger. Note that we can't use MAX() here as gcc doesn't allow that expansion inside of align(..).
Tested on omap3_beagle (which was previously broken) and a MAKEALL -a arm looks good too.
Signature · 1 line
Tom
PATCH 1/4 ehci-omap: Do not call dcache_off from omap_ehci_hcd_init
This has never been completely sufficient and now happens too late to paper over the cache coherency problems with the current USB stack.
--- drivers/usb/host/ehci-omap.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/host/ehci-omap.c +0 -1
@@ -246,7 +246,6 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata)if (is_ehci_phy_mode (usbhs_pdata->port_mode[i]))omap_ehci_soft_phy_reset (i); -dcache_disable (); hccr = (struct ehci_hccr *)(OMAP_EHCI_BASE ); hcor = (struct ehci_hcor *)(OMAP_EHCI_BASE +0x10 );
Signature · 1 line
1.7.9.5
PATCH 1/4 ehci-omap: Do not call dcache_off from omap_ehci_hcd_init
Dear Tom Rini,
Quoted 2 lines
This has never been completely sufficient and now happens too late to paper over the cache coherency problems with the current USB stack.
Poor USB maintainer isn't CCed :'-(
Quoted 2 lines
Signed-off-by Tom Rini <[email protected]>
But this is always a good thing to see.
Quoted 9 lines
--- drivers/usb/host/ehci-omap.c | 1 - 1 file changed, 1 deletion(-)drivers/usb/host/ehci-omap.c +0
-0
drivers/usb/host/ehci-omap.c int omap_ehci_hcd_init(struct omap_usbhs_board_data · line 250
Quoted 6 lines
drivers/usb/host/ehci-omap.c +0
-1@@ -246,7 +246,6 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata) if (is_ehci_phy_mode(usbhs_pdata->port_mode[i]))omap_ehci_soft_phy_reset (i); -dcache_disable (); hccr = (struct ehci_hccr *)(OMAP_EHCI_BASE ); hcor = (struct ehci_hcor *)(OMAP_EHCI_BASE +0x10 );
Best regards,
Marek Vasut
PATCH 1/4 ehci-omap: Do not call dcache_off from omap_ehci_hcd_init
Quoted 6 lines · Marek Vasut
Dear Tom Rini,
Quoted 2 lines
This has never been completely sufficient and now happens too late to paper over the cache coherency problems with the current USB stack.
Poor USB maintainer isn't CCed :'-(
Whoops, forgot. I don't know why I thought it was you :)
Quoted 26 lines
Quoted 3 lines
Signed-off-by Tom Rini <[email protected]>But this is always a good thing to see.
Acked-by Marek Vasut <[email protected]>Quoted 9 lines
--- drivers/usb/host/ehci-omap.c | 1 - 1 file changed, 1 deletion(-)drivers/usb/host/ehci-omap.c +0
-0drivers/usb/host/ehci-omap.c int omap_ehci_hcd_init(struct omap_usbhs_board_data · line 250
Quoted 6 lines
drivers/usb/host/ehci-omap.c +0
-1@@ -246,7 +246,6 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata) if (is_ehci_phy_mode(usbhs_pdata->port_mode[i]))omap_ehci_soft_phy_reset (i); -dcache_disable (); hccr = (struct ehci_hccr *)(OMAP_EHCI_BASE ); hcor = (struct ehci_hcor *)(OMAP_EHCI_BASE +0x10 );Best regards,
Marek Vasut
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
In reply to Tom RiniIf we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
--- arch/arm/cpu/armv7/cache_v7.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
arch/arm/cpu/armv7/cache_v7.c +11 -0
@@ -250,6 +250,9 @@ static void v7_inval_tlb(void)void invalidate_dcache_all (void ) { +if (!dcache_status ()) +return ; +v7_maint_dcache_all (ARMV7_DCACHE_INVAL_ALL );v7_outer_cache_inval_all ();
@@ -261,6 +264,9 @@ void invalidate_dcache_all(void)*/ void flush_dcache_all (void ) { +if (!dcache_status ()) +return ; +v7_maint_dcache_all (ARMV7_DCACHE_CLEAN_INVAL_ALL );v7_outer_cache_flush_all ();
@@ -272,6 +278,8 @@ void flush_dcache_all(void)*/ void invalidate_dcache_range (unsigned long start,unsigned long stop) { +if (!dcache_status ()) +return ;v7_dcache_maint_range (start, stop,ARMV7_DCACHE_INVAL_RANGE );
@@ -285,6 +293,9 @@ void invalidate_dcache_range(unsigned long start, unsigned long stop)*/ void flush_dcache_range (unsigned long start,unsigned long stop) { +if (!dcache_status ()) +return ; +v7_dcache_maint_range (start, stop,ARMV7_DCACHE_CLEAN_INVAL_RANGE );v7_outer_cache_flush_range (start, stop);
Signature · 1 line
1.7.9.5
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Dear Tom Rini,
Quoted 14 lines
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Signed-off-by Tom Rini <[email protected]>--- arch/arm/cpu/armv7/cache_v7.c | 11 +++++++++++ 1 file changed, 11 insertions(+)arch/arm/cpu/armv7/cache_v7.c +0
-0
arch/arm/cpu/armv7/cache_v7.c static void v7_inval_tlb(void) · line 254
Quoted 5 lines
arch/arm/cpu/armv7/cache_v7.c +2
-0@@ -250,6 +250,9 @@ static void v7_inval_tlb(void)void invalidate_dcache_all (void ) { +if (!dcache_status ()) +return ;
Will this get optimized out of the dcache is disabled altogether in uboot config?
btw this is 20% cooler in 10 seconds flat!
https://plus.google.com/102150693225130002912/posts/9gntjh57dXt
arch/arm/cpu/armv7/cache_v7.c static void v7_inval_tlb(void) · line 258
Quoted 4 lines
arch/arm/cpu/armv7/cache_v7.c +1
-0+v7_maint_dcache_all (ARMV7_DCACHE_INVAL_ALL );v7_outer_cache_inval_all ();
[...]
Best regards,
Marek Vasut
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Quoted 17 lines · Marek Vasut
Dear Tom Rini,
Quoted 15 lines
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Signed-off-by Tom Rini <[email protected]>--- arch/arm/cpu/armv7/cache_v7.c | 11 +++++++++++ 1 file changed, 11 insertions(+)arch/arm/cpu/armv7/cache_v7.c +0
-0@@ -250,6 +250,9 @@ static void v7_inval_tlb(void)
Quoted 7 lines
arch/arm/cpu/armv7/cache_v7.c static void v7_inval_tlb(void) · line 254
Quoted 4 lines
arch/arm/cpu/armv7/cache_v7.c +2
-0void invalidate_dcache_all (void ) { +if (!dcache_status ()) +return ;Will this get optimized out of the dcache is disabled altogether in uboot config?
That's another side of #ifs and that has empty functions.
Quoted 2 lines
btw this is 20% cooler in 10 seconds flat!
https://plus.google.com/102150693225130002912/posts/9gntjh57dXt
Ha.
Signature · 1 line
Tom
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Dear Tom Rini,
Quoted 22 lines · Marek Vasut
Quoted 21 lines
Dear Tom Rini,
Quoted 19 lines
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Signed-off-by Tom Rini <[email protected]>---
arch/arm/cpu/armv7/cache_v7.c | 11 +++++++++++ 1 file changed, 11 insertions(+)(quoted diff) +0
-0
b/arch/arm/cpu/armv7/cache_v7.cindex 1b4e808..1c0f5b0 100644arch/arm/cpu/armv7/cache_v7.c +0
-0@@ -250,6 +250,9 @@ static void v7_inval_tlb(void)void invalidate_dcache_all (void ) {
Quoted 7 lines
Quoted 5 lines
arch/arm/cpu/armv7/cache_v7.c static void v7_inval_tlb(void) · line 255
Quoted 2 lines
arch/arm/cpu/armv7/cache_v7.c +2
-0+if (!dcache_status ()) +return ;Will this get optimized out of the dcache is disabled altogether in uboot config?
That's another side of #ifs and that has empty functions.
Ok, that's good :-)
Quoted 5 lines
Quoted 3 lines
btw this is 20% cooler in 10 seconds flat!
https://plus.google.com/102150693225130002912/posts/9gntjh57dXtHa.
Best regards,
Marek Vasut
R
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
In reply to Tom RiniHi Tom,
Quoted 4 lines · Tom Rini
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
If it is a problem with unaligned regions, then that is the only
thing to be fixed
right ?. Just trying to understand why this change is required ?
Thanks,
Sricharan
Quoted 49 lines
Signed-off-by Tom Rini <[email protected]>---
?arch/arm/cpu/armv7/cache_v7.c | ? 11 +++++++++++
?1 file changed, 11 insertions(+)arch/arm/cpu/armv7/cache_v7.c +3
-0@@ -250,6 +250,9 @@ static void v7_inval_tlb(void) ?void invalidate_dcache_all(void) ?{ + ? ? ? if (!dcache_status()) + ? ? ? ? ? ? ? return; + ? ? ? ?v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); ? ? ? ?v7_outer_cache_inval_all();@@ -261,6 +264,9 @@ void invalidate_dcache_all(void)?*/
?voidflush_dcache_all(void)
?{
+ ? ? ? if (!dcache_status())
+ ? ? ? ? ? ? ? return;
+
? ? ? ?v7_maint_dcache_all(ARMV7_DCACHE_CLEAN_INVAL_ALL);? ? ? ?v7_outer_cache_flush_all();
arch/arm/cpu/armv7/cache_v7.c +0
-0@@ -272,6 +278,8 @@ void flush_dcache_all(void)?*/
?voidinvalidate_dcache_range(unsigned long start, unsigned long stop)?{
+ ? ? ? if (!dcache_status())
+ ? ? ? ? ? ? ? return;? ? ? ?v7_dcache_maint_range(start, stop,
ARMV7_DCACHE_INVAL_RANGE);arch/arm/cpu/armv7/cache_v7.c +0
-0@@ -285,6 +293,9 @@ void invalidate_dcache_range(unsigned long start, unsigned long stop)?*/
?voidflush_dcache_range(unsigned long start, unsigned long stop)?{
+ ? ? ? if (!dcache_status())
+ ? ? ? ? ? ? ? return;
+
? ? ? ?v7_dcache_maint_range(start, stop,ARMV7_DCACHE_CLEAN_INVAL_RANGE);? ? ? ?v7_outer_cache_flush_range(start, stop);
--
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Quoted 11 lines · Sricharan
Hi Tom,
Quoted 4 lines · Tom Rini
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
Quoted 3 lines
If it is a problem with unaligned regions, then that is the only
thing to be fixed
right ?. Just trying to understand why this change is required ?
The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
Signature · 1 line
Tom
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Dear Tom Rini,
Quoted 18 lines · Sricharan
Quoted 11 lines
Hi Tom,
Quoted 4 lines · Tom Rini
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
Quoted 2 lines
If it is a problem with unaligned regions, then that is the only
Quoted 8 lines
Quoted 3 lines
thing to be fixed
right ?. Just trying to understand why this change is required ?
The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
I think we should augment uboot to disallow tftp, fatload etc. to cache- unaligned address when caches are on ... this'd squash away the need for any shitty bounce buffers right away. Tom, will you be able to implement it, pretty please?
Of course, this doesn't squash the need for fixing FS code etc.
Best regards,
Marek Vasut
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Quoted 29 lines · Marek Vasut
Dear Tom Rini,
Quoted 18 lines · Sricharan
Quoted 11 lines
Hi Tom,
Quoted 4 lines · Tom Rini
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
Quoted 2 lines
If it is a problem with unaligned regions, then that is the only
Quoted 8 lines
Quoted 3 lines
thing to be fixed
right ?. Just trying to understand why this change is required ?
The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
Quoted 4 lines
I think we should augment uboot to disallow tftp, fatload etc. to cache- unaligned address when caches are on ... this'd squash away the need for any shitty bounce buffers right away. Tom, will you be able to implement it, pretty please?
But that's not the problem. The problem is all of the stuff going on within the USB/networking stack.
Signature · 1 line
Tom
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Dear Tom Rini,
Quoted 30 lines · Marek Vasut
Quoted 29 lines
Dear Tom Rini,
Quoted 26 lines · Sricharan
Quoted 11 lines
Hi Tom,
Quoted 4 lines · Tom Rini
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
Quoted 5 lines
If it is a problem with unaligned regions, then that is the only
thing to be fixed
right ?. Just trying to understand why this change is required ?
The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
Quoted 7 lines
Quoted 4 lines
I think we should augment uboot to disallow tftp, fatload etc. to cache- unaligned address when caches are on ... this'd squash away the need for any shitty bounce buffers right away. Tom, will you be able to implement it, pretty please?
But that's not the problem. The problem is all of the stuff going on within the USB/networking stack.
Correct. This will shield you from the users, I think it's worth to implement like that asap anyway. Or do we want a bounce buffer? I doubt that.
The other part is to fix the internals of u-boot. This will be the harder part.
Best regards,
Marek Vasut
R
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
In reply to Tom RiniHi,
Quoted 5 lines
Quoted 5 lines · Tom Rini
Quoted 4 lines
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Quoted 8 lines
Quoted 4 lines
? Today we do cache maintenance operations after the dcache is turned off. ? One example is before jumping to kernel, we try to invalidate the caches, ? in cache turned off state. So with this patch those maintenance calls will ? do nothing, which is not correct.
Ah yes, ?But, shouldn't we be doing these same operations as part of turning the cache off?
The problem is that while turning of dcaches, we flush it, and turn
cache and MMU off. But these operations are not happening automatically in a single call. So there is a chance of valid entries present in cache even after it is OFF.
So we invalidate it before jumping to kernel.
In fact our procedure of turning off the caches and MMU should have a relook, to really ensure things are safe. I will check on this more.
Quoted 9 lines
Quoted 3 lines
? ?If it is a problem with unaligned regions, then that is the only thing to be fixed
? right ?. Just trying to understand why this change is required ?The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. ?Without this every '#' of a tftp gives a screen full of error printfs. ?So tftp'ing a kernel takes minutes, not seconds, to complete.
Ok,but when buffers are not aligned
isn't that you are going to have coherency problems ?
Thanks,
Sricharan
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Quoted 20 lines · Sricharan
Hi,
Quoted 5 lines
Quoted 5 lines · Tom Rini
Quoted 4 lines
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Quoted 8 lines
Quoted 4 lines
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
The problem is that while turning of dcaches, we flush it, and turn
cache and MMU off. But these operations are not happening automatically in a single call. So there is a chance of valid entries present in cache even after it is OFF.
Quoted 5 lines
So we invalidate it before jumping to kernel.
In fact our procedure of turning off the caches and MMU should have a relook, to really ensure things are safe. I will check on this more.
OK, thanks.
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
If it is a problem with unaligned regions, then that is the only
thing to be fixed
right ?. Just trying to understand why this change is required ?The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
Ok,but when buffers are not aligned
isn't that you are going to have coherency problems ?
Exactly. USB requires dcache to be turned off today. With it turned off it spams these messages making tftp to an aligned address take minutes because the console is busy printing all of these messages about flushes it didn't need to do. The alternative is to build omap4/omap5/etc with dcache disabled or USB disabled.
I think the answer here is we need to make sure that when we switch the cache off (dcache_off()) we're always doing the kind of flushing that we had been doing for the kernel.
Signature · 1 line
Tom
R
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Hi,
[snip..]
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
? ?If it is a problem with unaligned regions, then that is the only thing to be fixed
? right ?. Just trying to understand why this change is required ?The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. ?Without this every '#' of a tftp gives a screen full of error printfs. ?So tftp'ing a kernel takes minutes, not seconds, to complete.
Quoted 9 lines
Quoted 2 lines
?Ok,but when buffers are not aligned
? ?isn't that you are going to have coherency problems ?Exactly. ?USB requires dcache to be turned off today. ?With it turned off it spams these messages making tftp to an aligned address take minutes because the console is busy printing all of these messages about flushes it didn't need to do. ?The alternative is to build omap4/omap5/etc with dcache disabled or USB disabled.
Ok, but who is calling cache maintenance in your case after
turning off caches ?
Quoted 3 lines
I think the answer here is we need to make sure that when we switch the cache off (
dcache_off()) we're always doing the kind of flushing that we had been doing for the kernel.
Yes, and also unaligned buffers will always be a problem. There should be a way to address that as well.
Thanks,
Sricharan
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Quoted 14 lines · Sricharan
Hi,
[snip..]Quoted 12 lines
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
If it is a problem with unaligned regions, then that is the only
thing to be fixed
right ?. Just trying to understand why this change is required ?The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
Ok,but when buffers are not aligned
isn't that you are going to have coherency problems ?
Quoted 8 lines
Quoted 6 lines
Exactly. USB requires dcache to be turned off today. With it turned off it spams these messages making tftp to an aligned address take minutes because the console is busy printing all of these messages about flushes it didn't need to do. The alternative is to build omap4/omap5/etc with dcache disabled or USB disabled.
Ok, but who is calling cache maintenance in your case after
turning off caches ?
The USB stack currently.
Signature · 1 line
Tom
R
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Hi Tom,
Quoted 21 lines · Tom Rini
Quoted 20 lines · Sricharan
Hi,
[snip..]Quoted 18 lines
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
? ?If it is a problem with unaligned regions, then that is the only thing to be fixed
? right ?. Just trying to understand why this change is required ?The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. ?Without this every '#' of a tftp gives a screen full of error printfs. ?So tftp'ing a kernel takes minutes, not seconds, to complete.
?Ok,but when buffers are not aligned
? ?isn't that you are going to have coherency problems ?Exactly. ?USB requires dcache to be turned off today. ?With it turned off it spams these messages making tftp to an aligned address take minutes because the console is busy printing all of these messages about flushes it didn't need to do. ?The alternative is to build omap4/omap5/etc with dcache disabled or USB disabled.
Quoted 4 lines
Quoted 2 lines
? ? Ok, but who is calling cache maintenance in your case after ? ?turning off caches ?
The USB stack currently.
Ok, in that case if
1) Alignment issue has to be fixed USB stack, (or)
2) Better to turn off caches when say USB-EHCI is enabled ? (or)
3) Turn of caches until cache library is fixed so that we will not require any maintenance after turning it off. ( I will work on this some time, this week)
I am not sure which one to do, but definitely we should not prevent it from executing as in the above patch. This may lead to other issues.
Thanks,
Sricharan
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Quoted 24 lines · Sricharan
Hi Tom,
Quoted 21 lines · Tom Rini
Quoted 20 lines · Sricharan
Hi,
[snip..]Quoted 18 lines
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
If it is a problem with unaligned regions, then that is the only
thing to be fixed
right ?. Just trying to understand why this change is required ?The problem is that within the USB/network/filesystem stacks we have a lot of not cache safe alignments apparently. Without this every '#' of a tftp gives a screen full of error printfs. So tftp'ing a kernel takes minutes, not seconds, to complete.
Ok,but when buffers are not aligned
isn't that you are going to have coherency problems ?Exactly. USB requires dcache to be turned off today. With it turned off it spams these messages making tftp to an aligned address take minutes because the console is busy printing all of these messages about flushes it didn't need to do. The alternative is to build omap4/omap5/etc with dcache disabled or USB disabled.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Ok, but who is calling cache maintenance in your case after
turning off caches ?The USB stack currently.
Ok, in that case if
1) Alignment issue has to be fixed USB stack, (or)
This is the correct, long term solution.
Quoted 1 line
2) Better to turn off caches when say USB-EHCI is enabled ?
Doing that is what showed the issue to start with. Running 'dcache off' doesn't turn off attempted cache flushing.
Quoted 5 lines
(or)3) Turn of caches until cache library is fixed so that we will not require any maintenance after turning it off. ( I will work on this some time, this week)
This is also a good thing. It just needs to make sure attempted flushes don't spam the console, when cache is off :)
Quoted 3 lines
I am not sure which one to do, but definitely we should not prevent it from executing as in the above patch. This may lead to other issues.
OK. I'll see how badly boot time goes up when dcache is off to start with on a few boards.
Signature · 1 line
Tom
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
In reply to RHi Sricharan,
Quoted 12 lines · Sricharan
Hi,
Quoted 10 lines
Quoted 9 lines · Tom Rini
Quoted 4 lines
If we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is turned off. One example is before jumping to kernel, we try to invalidate the caches, in cache turned off state. So with this patch those maintenance calls will do nothing, which is not correct.
Quoted 7 lines
Quoted 3 lines
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
The problem is that while turning of dcaches, we flush it, and turn
cache and MMU off. But these operations are not happening automatically in a single call. So there is a chance of valid entries present in cache even after it is OFF.
I think this is what we need to fix. Otherwise, Tom's change looks good to me. How about an invalidate in dcache_disable() or something like that?
br,
Aneesh
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Hi,
[snip..]
Quoted 22 lines · Sricharan
Quoted 4 lines
Hi,
Quoted 2 lines
Quoted 2 lines · Tom Rini
Quoted 1 line
If we are built with D-CACHE enabled but have run 'dcache off' and
then
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
attempt to flush unaligned regions we spam the console with
problems
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is
turned off.
Quoted 1 line
Quoted 1 line
Quoted 1 line
One example is before jumping to kernel, we try to invalidate
the caches,
Quoted 1 line
Quoted 1 line
Quoted 1 line
in cache turned off state. So with this patch those maintenance
calls will
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
The problem is that while turning of dcaches, we flush it, and
Quoted 9 lines
turn
Quoted 3 lines
cache and MMU off. But these operations are not happening automatically in a single call. So there is a chance of valid entries present in cache even after it is OFF.
I think this is what we need to fix. Otherwise, Tom's change looks good to me. How about an invalidate in
dcache_disable()or something like that?
Correct. So if the cleaning up sequence is fine, then ok.
So there should be no need to check if caches are OFF inside the maintenance functions. Kernel does not looks like doing such checks. The real issue looks like we should have had assembly level functions to do the cleanup routines for flush-invalidate-turn OFF caches/MMU
atomically.
Thanks,
Sricharan
R
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Hi Aneesh,
Quoted 33 lines · Sricharan R
Hi,
[snip..]Quoted 22 lines · Sricharan
Quoted 4 lines
Hi,
Quoted 2 lines
Quoted 2 lines · On Fri, Jun 15, 2012 at 12:31 AM, Tom Rini<[email protected]> ?wrote:
Quoted 1 line
If we are built with D-CACHE enabled but have run 'dcache off' and
then
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
attempt to flush unaligned regions we spam the console with
problems
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
that aren't true (as the cache was off).
? ?Today we do cache maintenance operations after the dcache is
turned off.
Quoted 1 line
Quoted 1 line
Quoted 1 line
? ?One example is before jumping to kernel, we try to invalidate
the caches,
Quoted 1 line
Quoted 1 line
Quoted 1 line
? ?in cache turned off state. So with this patch those maintenance
calls will
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
? ?do nothing, which is not correct.
Ah yes, ?But, shouldn't we be doing these same operations as part of turning the cache off?
? ?The problem is that while turning of dcaches, we flush it, and
Quoted 9 lines
turn
Quoted 3 lines
? cache and MMU off. ?But these operations are not happening ? automatically in a single call. So there is a chance of ?valid ? entries present in cache even after it is OFF.
I think this is what we need to fix. Otherwise, Tom's change looks good to me. How about an invalidate in
dcache_disable()or something like that?
Quoted 6 lines
?Correct. So if the cleaning up sequence is fine, then ok. ?So there should be no need to check if caches are OFF inside ?the maintenance functions. Kernel does not looks like doing such checks. ?The real issue looks like we should have had assembly level functions ?to do the cleanup routines for flush-invalidate-turn OFF caches/MM atomically.
Actually, with something like speculation in the behind, only way of
ensuring that
we do not have any valid entries is to do a invalidate all, after
turn of caches/mmu.
So this means that we will have a maintenance after turning off.
Thanks,
Sricharan
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Sricharan,
Quoted 42 lines · Sricharan
Hi Aneesh,
Quoted 33 lines · Sricharan R
Hi,
[snip..]Quoted 22 lines · Sricharan
Quoted 4 lines
Hi,
Quoted 2 lines
Quoted 2 lines · Tom Rini
Quoted 1 line
If we are built with D-CACHE enabled but have run 'dcache off' and
then
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
attempt to flush unaligned regions we spam the console with
problems
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is
turned off.
Quoted 1 line
Quoted 1 line
Quoted 1 line
One example is before jumping to kernel, we try to invalidate
the caches,
Quoted 1 line
Quoted 1 line
Quoted 1 line
in cache turned off state. So with this patch those maintenance
calls will
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
The problem is that while turning of dcaches, we flush it, and
Quoted 9 lines
turn
Quoted 3 lines
cache and MMU off. But these operations are not happening automatically in a single call. So there is a chance of valid entries present in cache even after it is OFF.
I think this is what we need to fix. Otherwise, Tom's change looks good to me. How about an invalidate in
dcache_disable()or something like that?Quoted 6 lines
Correct. So if the cleaning up sequence is fine, then ok.
So there should be no need to check if caches are OFF inside the maintenance functions. Kernel does not looks like doing such checks. The real issue looks like we should have had assembly level functions to do the cleanup routines for flush-invalidate-turn OFF caches/MM
atomically.
Quoted 5 lines
Actually, with something like speculation in the behind, only way of
ensuring that
we do not have any valid entries is to do a invalidate all, after
turn of caches/mmu.
So this means that we will have a maintenance after turning off.
Good point. But we need that invalidate just one last time after the disable, right? How about making the cache_status a variable rather than reading the C bit. And then disable_cache() shall be like this:
1. Flush all
2. Disable C bit
3. Invalidate all
4. cache_status = disabled
Maybe, not the best solution. But I can't think of anything better. Please note that after this point there won't be any valid entries in the cache per ARMv7 Architecture reference manual(section B2.2.2):
"If the cache is disabled, it is guaranteed that no new allocation of memory locations into the cache will occur."
br,
Aneeesh
R
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
Aneesh,
[snip..]
Quoted 64 lines
Quoted 48 lines
Quoted 41 lines
Quoted 32 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
If we are built with D-CACHE enabled but have run 'dcache off' and
then
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
attempt to flush unaligned regions we spam the console with
problems
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
that aren't true (as the cache was off).
? ?Today we do cache maintenance operations after the dcache is
turned off.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
? ?One example is before jumping to kernel, we try to invalidate
the caches,
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
? ?in cache turned off state. So with this patch those maintenance
calls will
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
? ?do nothing, which is not correct.
Ah yes, ?But, shouldn't we be doing these same operations as part of turning the cache off?
? ?The problem is that while turning of dcaches, we flush it, and
turn
Quoted 1 line
Quoted 9 lines
Quoted 3 lines
? cache and MMU off. ?But these operations are not happening ? automatically in a single call. So there is a chance of ?valid ? entries present in cache even after it is OFF.
I think this is what we need to fix. Otherwise, Tom's change looks good to me. How about an invalidate in
dcache_disable()or something like that?Quoted 6 lines
?Correct. So if the cleaning up sequence is fine, then ok. ?So there should be no need to check if caches are OFF inside ?the maintenance functions. Kernel does not looks like doing such checks. ?The real issue looks like we should have had assembly level functions ?to do the cleanup routines for flush-invalidate-turn OFF caches/MM atomically.
Quoted 5 lines
? Actually, with something like speculation in the behind, only way of ensuring that
? ?we do not have any valid entries is to do a invalidate all, after turn of caches/mmu.
? ?So this means that we will have a maintenance after turning off.Good point. But we need that invalidate just one last time after the disable, right? How about making the
cache_statusa variable rather than reading the C bit. And thendisable_cache()shall be like this:1. Flush all
2. Disable C bit
3. Invalidate all
4.cache_status= disabled
Quoted 6 lines
Maybe, not the best solution. But I can't think of anything better. Please note that after this point there won't be any valid entries in the cache per ARMv7 Architecture reference manual(section B2.2.2):
"If the cache is disabled, it is guaranteed that no new allocation of memory locations into the cache will occur."
I agree on this sequence. But the only thing is, is it good to turn off the caches runtime, because one driver cant do aligned access ?
Will this then not become a custom, say any other driver which sees problem with cache, will then simply go ahead disable and proceed.
Is that right ?.
yes, having enabled caches we are missing things like UNCACHED allocations, IO etc, but that is going to make boot loaders more heavy, may be.
In the end, I am not sure which is the right decision to take here :)
Thanks,
Sricharan
PATCH 2/4 cache_v7: Check for dcache enablement in dcache flush functions
In reply to Sricharan RHi Sricharan,
Quoted 33 lines · Sricharan R
Hi,
[snip..]Quoted 22 lines · Sricharan
Quoted 4 lines
Hi,
Quoted 2 lines
Quoted 2 lines · Tom Rini
Quoted 1 line
If we are built with D-CACHE enabled but have run 'dcache off' and
then
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
attempt to flush unaligned regions we spam the console with
problems
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
that aren't true (as the cache was off).
Today we do cache maintenance operations after the dcache is
turned off.
Quoted 1 line
Quoted 1 line
Quoted 1 line
One example is before jumping to kernel, we try to invalidate
the caches,
Quoted 1 line
Quoted 1 line
Quoted 1 line
in cache turned off state. So with this patch those maintenance
calls will
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
do nothing, which is not correct.
Ah yes, But, shouldn't we be doing these same operations as part of turning the cache off?
The problem is that while turning of dcaches, we flush it, and
Quoted 9 lines
turn
Quoted 3 lines
cache and MMU off. But these operations are not happening automatically in a single call. So there is a chance of valid entries present in cache even after it is OFF.
I think this is what we need to fix. Otherwise, Tom's change looks good to me. How about an invalidate in
dcache_disable()or something like that?
Quoted 6 lines
Correct. So if the cleaning up sequence is fine, then ok.
So there should be no need to check if caches are OFF inside the maintenance functions. Kernel does not looks like doing such checks. The real issue looks like we should have had assembly level functions to do the cleanup routines for flush-invalidate-turn OFF caches/MMU
atomically.
Sorry for the late reply. Yes, kernel doesn't do such checks. But kernel's implementation is different from u-boot implementation. In kernel the invalidate routine flushes unaligned lines in the end which, after a lot of discussions[1], we decided to avoid in u-boot. Instead we printed noisy error messages to alert the user. Now these messages will appear even if we are calling the maintenance functions after the disable and that's what Tom is trying to avoid.
br,
Aneeesh
[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/105113
PATCH 3/4 ehci-hcd.c: Add a new USB_DMA_MINALIGN define for cache alignment
In reply to Tom RiniThe USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
--- drivers/usb/host/ehci-hcd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
drivers/usb/host/ehci-hcd.c +17 -6
@@ -29,12 +29,23 @@#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif + +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN))); static struct descriptor { struct usb_hub_descriptor hub;
@@ -207,8 +218,8 @@ static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) { - static struct QH qh __attribute__((aligned(32))); - static struct qTD qtd[3] __attribute__((aligned (32))); + static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN))); + static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));int qtd_counter =0 ;volatile struct qTD *vtd;
Signature · 1 line
1.7.9.5
PATCH 3/4 ehci-hcd.c: Add a new USB_DMA_MINALIGN define for cache alignment
Dear Tom Rini,
Quoted 21 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--- drivers/usb/host/ehci-hcd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/usb/host/ehci-hcd.c +0
-2@@ -29,12 +29,23 @@#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */
drivers/usb/host/ehci-hcd.c line 40
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +9
-1-volatile struct ehci_hcor *hcor; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif
Don't we have some common header for these?
Quoted 14 lines
drivers/usb/host/ehci-hcd.c +6
-1+ +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN))); static struct descriptor {drivers/usb/host/ehci-hcd.c +0
-0struct usb_hub_descriptor hub;
drivers/usb/host/ehci-hcd.c static int · line 225
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +2
-2@@ -207,8 +218,8 @@ static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) { - static struct QH qh __attribute__((aligned(32))); - static struct qTD qtd[3] __attribute__((aligned (32))); + static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN))); + static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));int qtd_counter =0 ;volatile struct qTD *vtd;
Best regards,
Marek Vasut
PATCH 3/4 ehci-hcd.c: Add a new USB_DMA_MINALIGN define for cache alignment
Quoted 21 lines · Marek Vasut
Dear Tom Rini,
Quoted 19 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--- drivers/usb/host/ehci-hcd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,23 @@#include "ehci.h"
Quoted 14 lines
drivers/usb/host/ehci-hcd.c line 40
Quoted 12 lines
drivers/usb/host/ehci-hcd.c +9
-3-int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif Don't we have some common header for these?
For ECHI and musb? I did not spot one unless we go all the way up to common.h or similar.
Signature · 1 line
Tom
PATCH 3/4 ehci-hcd.c: Add a new USB_DMA_MINALIGN define for cache alignment
Dear Tom Rini,
Quoted 38 lines · Marek Vasut
Quoted 22 lines
Dear Tom Rini,
Quoted 20 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>---
drivers/usb/host/ehci-hcd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,23 @@#include "ehci.h" Quoted 14 lines
drivers/usb/host/ehci-hcd.c line 40
Quoted 12 lines
drivers/usb/host/ehci-hcd.c +2
-3-int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, + * some platforms require larger alignment.
+ */drivers/usb/host/ehci-hcd.c +5
-0+#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif Don't we have some common header for these?
Quoted 2 lines
For ECHI and musb? I did not spot one unless we go all the way up to
common.hor similar.
Ok, that's crappy :-/
Don't we have ehci.h or usb.h? Is musb ehci or not?
Best regards,
Marek Vasut
PATCH 3/4 ehci-hcd.c: Add a new USB_DMA_MINALIGN define for cache alignment
Quoted 40 lines · Marek Vasut
Dear Tom Rini,
Quoted 38 lines · Marek Vasut
Quoted 36 lines
Dear Tom Rini,
Quoted 27 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>---
drivers/usb/host/ehci-hcd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/usb/host/ehci-hcd.c +2
-3@@ -29,12 +29,23 @@#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, + * some platforms require larger alignment.
+ */drivers/usb/host/ehci-hcd.c line 40
Quoted 5 lines
drivers/usb/host/ehci-hcd.c +5
-0+#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif Don't we have some common header for these?
Quoted 6 lines
Quoted 2 lines
For ECHI and musb? I did not spot one unless we go all the way up to
common.hor similar.Ok, that's crappy :-/
Don't we have
ehci.horusb.h? Is musb ehci or not?
MUSB is Mentor USB. But, good spotting, include/usb.h is in both ehci-hcd.c (and other places) and musb_core.h, moving there.
Signature · 1 line
Tom
PATCH 3/4 ehci-hcd.c: Add a new USB_DMA_MINALIGN define for cache alignment
Dear Tom Rini,
Quoted 44 lines · Marek Vasut
Quoted 43 lines
Dear Tom Rini,
Quoted 40 lines · Marek Vasut
Quoted 22 lines
Dear Tom Rini,
Quoted 20 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>---
drivers/usb/host/ehci-hcd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,23 @@#include "ehci.h" Quoted 14 lines
drivers/usb/host/ehci-hcd.c line 40
Quoted 12 lines
drivers/usb/host/ehci-hcd.c +2
-3-int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, + * some platforms require larger alignment.
+ */drivers/usb/host/ehci-hcd.c +5
-0+#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif Don't we have some common header for these?
For ECHI and musb? I did not spot one unless we go all the way up to
common.hor similar.
Quoted 6 lines
Quoted 3 lines
Ok, that's crappy :-/
Don't we have
ehci.horusb.h? Is musb ehci or not?MUSB is Mentor USB. But, good spotting,
include/usb.his in bothehci-hcd.c(and other places) andmusb_core.h, moving there.
Heh, my clairvoiance can now do git-grep-alike actions without touching the repo ... I have to praise myself :-)
Best regards,
Marek Vasut
PATCH 4/4 musb_core.h: Add a new USB_DMA_MINALIGN define for cache alignment
In reply to Tom RiniThe USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum.
--- drivers/usb/musb/musb_core.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/usb/musb/musb_core.h +11 -1
@@ -74,6 +74,16 @@ struct musb_epN_regs {u8 fifosize; }; +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif +/* Mentor USB core register overlay structure */ #ifndef musb_regs struct musb_regs {
@@ -145,7 +155,7 @@ struct musb_regs { struct musb_epN_regs epN; } ep[16 ]; -} __attribute__((packed, aligned(32))); +} __attribute__((packed, aligned(USB_DMA_MINALIGN)));#endif /*
Signature · 1 line
1.7.9.5
v2 cover USB and cache related fixes
In reply to Tom RiniHey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32). First, the dcache_off call that ehci-omap had been doing was now not happening soon enough to paper over the cache issues. This call is removed in patch 1. Second, when we have dcache support compiled in but turned off via 'dcache off' the cache routines spam the console about alignment issues when a cache flush is attempted. This is a problem in that it makes operations extremely slow (as we're spending all our time spitting messages to console). The second patch makes the flush routines return when the dcache is off. The last patch deal with the same problem, for EHCI and for MUSB. The USB spec says that 32 bytes is the minimum alignment but we need larger alignment when the cache is larger. Note that we can't use MAX() here as gcc doesn't allow that expansion inside of align(..).
Tested on omap3_beagle (which was previously broken) and a MAKEALL -a arm looks good too.
Changes in v2:
- Condense last two patches into one that puts the test into <usb.h>
Signature · 1 line
Tom
PATCH v2 1/3 ehci-omap: Do not call dcache_off from omap_ehci_hcd_init
This has never been completely sufficient and now happens too late to paper over the cache coherency problems with the current USB stack.
--- drivers/usb/host/ehci-omap.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/host/ehci-omap.c +0 -1
@@ -246,7 +246,6 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata)if (is_ehci_phy_mode (usbhs_pdata->port_mode[i]))omap_ehci_soft_phy_reset (i); -dcache_disable (); hccr = (struct ehci_hccr *)(OMAP_EHCI_BASE ); hcor = (struct ehci_hcor *)(OMAP_EHCI_BASE +0x10 );
Signature · 1 line
1.7.9.5
PATCH v2 2/3 cache_v7: Check for dcache enablement in dcache flush functions
In reply to Tom RiniIf we are built with D-CACHE enabled but have run 'dcache off' and then attempt to flush unaligned regions we spam the console with problems that aren't true (as the cache was off).
--- arch/arm/cpu/armv7/cache_v7.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
arch/arm/cpu/armv7/cache_v7.c +11 -0
@@ -250,6 +250,9 @@ static void v7_inval_tlb(void)void invalidate_dcache_all (void ) { +if (!dcache_status ()) +return ; +v7_maint_dcache_all (ARMV7_DCACHE_INVAL_ALL );v7_outer_cache_inval_all ();
@@ -261,6 +264,9 @@ void invalidate_dcache_all(void)*/ void flush_dcache_all (void ) { +if (!dcache_status ()) +return ; +v7_maint_dcache_all (ARMV7_DCACHE_CLEAN_INVAL_ALL );v7_outer_cache_flush_all ();
@@ -272,6 +278,8 @@ void flush_dcache_all(void)*/ void invalidate_dcache_range (unsigned long start,unsigned long stop) { +if (!dcache_status ()) +return ;v7_dcache_maint_range (start, stop,ARMV7_DCACHE_INVAL_RANGE );
@@ -285,6 +293,9 @@ void invalidate_dcache_range(unsigned long start, unsigned long stop)*/ void flush_dcache_range (unsigned long start,unsigned long stop) { +if (!dcache_status ()) +return ; +v7_dcache_maint_range (start, stop,ARMV7_DCACHE_CLEAN_INVAL_RANGE );v7_outer_cache_flush_range (start, stop);
Signature · 1 line
1.7.9.5
PATCH v2 3/3 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
In reply to Tom RiniThe USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <usb.h> to define USB_DMA_MINALIGN and make use of it in ehci-hcd.c and musb_core.h. We cannot use MAX() here as we are not allowed to have tests inside of align(...).
--
Changes in v2:
- Move test to <usb.h>, expand comment.
--- drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)
drivers/usb/host/ehci-hcd.c +7 -6
@@ -29,12 +29,13 @@#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN))); static struct descriptor { struct usb_hub_descriptor hub;
@@ -207,8 +208,8 @@ static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) { - static struct QH qh __attribute__((aligned(32))); - static struct qTD qtd[3] __attribute__((aligned (32))); + static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN))); + static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));int qtd_counter =0 ;volatile struct qTD *vtd;
drivers/usb/musb/musb_core.h +1 -1
@@ -145,7 +145,7 @@ struct musb_regs { struct musb_epN_regs epN; } ep[16 ]; -} __attribute__((packed, aligned(32))); +} __attribute__((packed, aligned(USB_DMA_MINALIGN)));#endif /*
include/usb.h +10 -0
@@ -29,6 +29,16 @@#include <usb_defs.h> #include <usbdescriptors.h> +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif +/* Everything is aribtrary */ #define USB_ALTSETTINGALLOC 4#define USB_MAXALTSETTING 128/* Hard limit */
Signature · 1 line
1.7.9.5
v3 cover USB and cache related fixes
In reply to Tom RiniHey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32). First, the dcache_off call that ehci-omap had been doing was now not happening soon enough to paper over the cache issues. This call is removed in patch 1. The second patch deal with the same problem in EHCI and MUSB. The USB spec says that 32 bytes is the minimum alignment but we need larger alignment when the cache is larger. Note that we can't use MAX() here as gcc doesn't allow that expansion inside of align(..). Patches 3 to 6 disable dcache support at build time on all platforms that enable CONFIG_USB_EHCI_OMAP because performing a run-time 'dcache off' operation leaves USB unusable due to the unaligned flushes that are still attempted. Run-time testing on an omap3_beagle shows a very slight (less than half a second, checked with grabserial) increase in load time for a 3.5-rc3 kernel image from SD card. Note that otherwise a tftp load takes minutes to complete rather than seconds due to all of the console spam.
Tested on omap3_beagle (which was previously broken) and a MAKEALL -a arm looks good too.
Changes in v3:
- Drop the cache_v7.c change and instead make all CONFIG_USB_EHCI_OMAP boards disable DCACHE at build time.
Changes in v2:
- Condense last two patches into one that puts the test into <usb.h>
Signature · 1 line
Tom
PATCH v3 1/6 ehci-omap: Do not call dcache_off from omap_ehci_hcd_init
This has never been completely sufficient and now happens too late to paper over the cache coherency problems with the current USB stack.
--- drivers/usb/host/ehci-omap.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/host/ehci-omap.c +0 -1
@@ -246,7 +246,6 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata)if (is_ehci_phy_mode (usbhs_pdata->port_mode[i]))omap_ehci_soft_phy_reset (i); -dcache_disable (); hccr = (struct ehci_hccr *)(OMAP_EHCI_BASE ); hcor = (struct ehci_hcor *)(OMAP_EHCI_BASE +0x10 );
Signature · 1 line
1.7.9.5
PATCH v3 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
In reply to Tom RiniThe USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <usb.h> to define USB_DMA_MINALIGN and make use of it in ehci-hcd.c and musb_core.h. We cannot use MAX() here as we are not allowed to have tests inside of align(...).
--
Changes in v2:
- Move test to <usb.h>, expand comment.
--- drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)
drivers/usb/host/ehci-hcd.c +7 -6
@@ -29,12 +29,13 @@#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN))); static struct descriptor { struct usb_hub_descriptor hub;
@@ -207,8 +208,8 @@ static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) { - static struct QH qh __attribute__((aligned(32))); - static struct qTD qtd[3] __attribute__((aligned (32))); + static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN))); + static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));int qtd_counter =0 ;volatile struct qTD *vtd;
drivers/usb/musb/musb_core.h +1 -1
@@ -145,7 +145,7 @@ struct musb_regs { struct musb_epN_regs epN; } ep[16 ]; -} __attribute__((packed, aligned(32))); +} __attribute__((packed, aligned(USB_DMA_MINALIGN)));#endif /*
include/usb.h +10 -0
@@ -29,6 +29,16 @@#include <usb_defs.h> #include <usbdescriptors.h> +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif +/* Everything is aribtrary */ #define USB_ALTSETTINGALLOC 4#define USB_MAXALTSETTING 128/* Hard limit */
Signature · 1 line
1.7.9.5
PATCH v3 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
Dear Tom Rini,
Quoted 25 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <
usb.h> to defineUSB_DMA_MINALIGNand make use of it inehci-hcd.candmusb_core.h. We cannot useMAX()here as we are not allowed to have tests inside of align(...).Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--
Changes in v2:
- Move test to <usb.h>, expand comment.--- drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,13 @@
drivers/usb/host/ehci-hcd.c line 36
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +4
-3#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));
^ these need to be aligned too?
Quoted 42 lines
drivers/usb/host/ehci-hcd.c +3
-3static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN))); static struct descriptor { struct usb_hub_descriptor hub;@@ -207,8 +208,8 @@ static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) { - static struct QH qh __attribute__((aligned(32))); - static struct qTD qtd[3] __attribute__((aligned (32))); + static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN))); + static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));int qtd_counter =0 ;volatile struct qTD *vtd;drivers/usb/musb/musb_core.h +1
-1@@ -145,7 +145,7 @@ struct musb_regs { struct musb_epN_regs epN; } ep[16 ]; -} __attribute__((packed, aligned(32))); +} __attribute__((packed, aligned(USB_DMA_MINALIGN)));#endif /* include/usb.h +3
-0@@ -29,6 +29,16 @@#include <usb_defs.h> #include <usbdescriptors.h> +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment.
include/usb.h line 44
Quoted 10 lines
include/usb.h +7
-0+*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif +/* Everything is aribtrary */ #define USB_ALTSETTINGALLOC 4#define USB_MAXALTSETTING 128/* Hard limit */
Best regards,
Marek Vasut
PATCH v3 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
Quoted 29 lines · Marek Vasut
Dear Tom Rini,
Quoted 27 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <
usb.h> to defineUSB_DMA_MINALIGNand make use of it inehci-hcd.candmusb_core.h. We cannot useMAX()here as we are not allowed to have tests inside of align(...).Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--
Changes in v2:
- Move test to <usb.h>, expand comment.--- drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,13 @@#include "ehci.h"
Quoted 10 lines
drivers/usb/host/ehci-hcd.c
Quoted 8 lines
drivers/usb/host/ehci-hcd.c +4
-3-int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));^ these need to be aligned too?
Yes, these were the first and easy to spot ones in the cache flush messages, I would swear.
Signature · 1 line
Tom
PATCH v3 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
Dear Tom Rini,
Quoted 43 lines · Marek Vasut
Quoted 31 lines
Dear Tom Rini,
Quoted 29 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <
usb.h> to defineUSB_DMA_MINALIGNand make use of it inehci-hcd.candmusb_core.h. We cannot useMAX()here as we are not allowed to have tests inside of align(...).Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--
Changes in v2:
- Move test to <usb.h>, expand comment.
---drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,13 @@#include "ehci.h" Quoted 10 lines
drivers/usb/host/ehci-hcd.c
Quoted 8 lines
drivers/usb/host/ehci-hcd.c +4
-3-int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));^ these need to be aligned too?
Quoted 2 lines
Yes, these were the first and easy to spot ones in the cache flush messages, I would swear.
hcor and hccr are register locations though ... so it's pretty weird.
Best regards,
Marek Vasut
PATCH v3 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
Quoted 45 lines · Marek Vasut
Dear Tom Rini,
Quoted 43 lines · Marek Vasut
Quoted 41 lines
Dear Tom Rini,
Quoted 27 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <
usb.h> to defineUSB_DMA_MINALIGNand make use of it inehci-hcd.candmusb_core.h. We cannot useMAX()here as we are not allowed to have tests inside of align(...).Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--
Changes in v2:
- Move test to <usb.h>, expand comment.
---drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,13 @@drivers/usb/host/ehci-hcd.c line 36
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +4
-3#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile */ -volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));^ these need to be aligned too?
Quoted 4 lines
Quoted 2 lines
Yes, these were the first and easy to spot ones in the cache flush messages, I would swear.
hcor and hccr are register locations though ... so it's pretty weird.
OK, now that I look harder at the messages, doing that is only masking a problem. At issue (from u-boot.map):
.bss 0x801586c0 0x1c0 drivers/usb/host/libusb_host.o
0x80158860 hcor
And a message of:
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x9ffbc860 Using 'bdinfo' to see reloc offset gives us hcor, but _end_ not start means it's something within the anonymous part of the bss and padding hcor out just masks the real problem. I'll resubmit a v4 shortly. Thanks!
Signature · 1 line
Tom
PATCH v3 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
Dear Tom Rini,
Quoted 33 lines · Marek Vasut
Quoted 32 lines
Dear Tom Rini,
Quoted 30 lines · Marek Vasut
Quoted 29 lines
Dear Tom Rini,
Quoted 27 lines
The USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <
usb.h> to defineUSB_DMA_MINALIGNand make use of it inehci-hcd.candmusb_core.h. We cannot useMAX()here as we are not allowed to have tests inside of align(...).Cc Marek Vasut <[email protected]>Signed-off-by Tom Rini <[email protected]>--
Changes in v2:
- Move test to <usb.h>, expand comment.
---drivers/usb/host/ehci-hcd.c | 13 +++++++------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-)(quoted diff) +0
-0
b/drivers/usb/host/ehci-hcd.cindex 04300be..5a86117 100644drivers/usb/host/ehci-hcd.c +0
-0@@ -29,12 +29,13 @@
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/usb/host/ehci-hcd.c
Quoted 4 lines
drivers/usb/host/ehci-hcd.c +0
-2#include "ehci.h" -int rootdev; -struct ehci_hccr *hccr;/* R/O registers, not need for volatile
*/
Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/usb/host/ehci-hcd.c
Quoted 6 lines
drivers/usb/host/ehci-hcd.c +4
-1-volatile struct ehci_hcor *hcor; +int rootdev __attribute__((aligned(USB_DMA_MINALIGN))); +/* R/O registers, not need for volatile */ +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN))); +volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));^ these need to be aligned too?
Quoted 4 lines
Quoted 2 lines
Yes, these were the first and easy to spot ones in the cache flush messages, I would swear.
hcor and hccr are register locations though ... so it's pretty weird.
OK, now that I look harder at the messages, doing that is only masking a problem. At issue (from u-boot.map):
.bss0x801586c00x1c0drivers/usb/host/libusb_host.o
Quoted 7 lines
0x80158860 hcorAnd a message of:
ERROR:v7_dcache_inval_range- stop address is not aligned -0x9ffbc860Using 'bdinfo' to see reloc offset gives us hcor, but _end_ not start means it's something within the anonymous part of the bss and padding hcor out just masks the real problem. I'll resubmit a v4 shortly. Thanks!
You're welcome, Add my Acked-by: Marek Vasut <[email protected]> and push through omap since it's part of a bigger patchset.
Best regards,
Marek Vasut
PATCH v3 3/6 mcx: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/mcx.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/configs/mcx.h +1 -1
@@ -110,9 +110,9 @@#define CONFIG_OMAP3_GPIO_5 #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_ULPI #define CONFIG_USB_ULPI_VIEWPORT_OMAP -/*#define CONFIG_EHCI_DCACHE*/ /* leave it disabled for now */ #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 154#define CONFIG_OMAP_EHCI_PHY2_RESET_GPIO 152#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
Signature · 1 line
1.7.9.5
PATCH v3 4/6 omap3_beagle: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/omap3_beagle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/configs/omap3_beagle.h +1 -1
@@ -131,7 +131,7 @@#define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP -/*#define CONFIG_EHCI_DCACHE*/ /* leave it disabled for now */ +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 147#define CONFIG_USB_ULPI
Signature · 1 line
1.7.9.5
PATCH v3 5/6 omap4_panda: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/omap4_panda.h | 1 + 1 file changed, 1 insertion(+)
include/configs/omap4_panda.h +1 -0
@@ -38,6 +38,7 @@#define CONFIG_USB_HOST #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_STORAGE #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
Signature · 1 line
1.7.9.5
PATCH v3 6/6 tam3517-common: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/tam3517-common.h | 1 + 1 file changed, 1 insertion(+)
include/configs/tam3517-common.h +1 -0
@@ -100,6 +100,7 @@#define CONFIG_OMAP3_GPIO_5 #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_ULPI #define CONFIG_USB_ULPI_VIEWPORT_OMAP #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 25
Signature · 1 line
1.7.9.5
v4 cover USB and cache related fixes
In reply to Tom RiniHey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32). First, the dcache_off call that ehci-omap had been doing was now not happening soon enough to paper over the cache issues. This call is removed in patch 1. The second patch deal with the same problem in EHCI and MUSB. The USB spec says that 32 bytes is the minimum alignment but we need larger alignment when the cache is larger. Note that we can't use MAX() here as gcc doesn't allow that expansion inside of align(..). Patches 3 to 6 disable dcache support at build time on all platforms that enable CONFIG_USB_EHCI_OMAP because performing a run-time 'dcache off' operation leaves USB unusable due to the unaligned flushes that are still attempted. Run-time testing on an omap3_beagle shows a very slight (less than half a second, checked with grabserial) increase in load time for a 3.5-rc3 kernel image from SD card. Note that otherwise a tftp load takes minutes to complete rather than seconds due to all of the console spam.
Tested on omap3_beagle (which was previously broken) and a MAKEALL -a arm looks good too.
Changes in v4:
- Drop alignment on rootdev/hccr/hcor as a double-check showed that was masking an alignment problem elsewhere (Marek Vasut)
Changes in v3:
- Drop the cache_v7.c change and instead make all CONFIG_USB_EHCI_OMAP boards disable DCACHE at build time.
Changes in v2:
- Condense last two patches into one that puts the test into <usb.h>
Signature · 1 line
Tom
PATCH v4 1/6 ehci-omap: Do not call dcache_off from omap_ehci_hcd_init
This has never been completely sufficient and now happens too late to paper over the cache coherency problems with the current USB stack.
--- drivers/usb/host/ehci-omap.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/host/ehci-omap.c +0 -1
@@ -246,7 +246,6 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata)if (is_ehci_phy_mode (usbhs_pdata->port_mode[i]))omap_ehci_soft_phy_reset (i); -dcache_disable (); hccr = (struct ehci_hccr *)(OMAP_EHCI_BASE ); hcor = (struct ehci_hcor *)(OMAP_EHCI_BASE +0x10 );
Signature · 1 line
1.7.9.5
PATCH v4 2/6 ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment
In reply to Tom RiniThe USB spec says that 32 bytes is the minimum required alignment. However on some platforms we have a larger minimum requirement for cache coherency. In those cases, use that value rather than the USB spec minimum. We add a cpp check to <usb.h> to define USB_DMA_MINALIGN and make use of it in ehci-hcd.c and musb_core.h. We cannot use MAX() here as we are not allowed to have tests inside of align(...).
--
Changes in v4:
- Re-checking shows we do not need to add alignment to rootdev/hcor/hccr
Changes in v2:
- Move test to <usb.h>, expand comment.
--- drivers/usb/host/ehci-hcd.c | 6 +++--- drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++++++ 3 files changed, 14 insertions(+), 4 deletions(-)
drivers/usb/host/ehci-hcd.c +3 -3
@@ -34,7 +34,7 @@ struct ehci_hccr *hccr; /* R/O registers, not need for volatile */volatile struct ehci_hcor *hcor;static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN))); static struct descriptor { struct usb_hub_descriptor hub;
@@ -207,8 +207,8 @@ static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) { - static struct QH qh __attribute__((aligned(32))); - static struct qTD qtd[3] __attribute__((aligned (32))); + static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN))); + static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));int qtd_counter =0 ;volatile struct qTD *vtd;
drivers/usb/musb/musb_core.h +1 -1
@@ -145,7 +145,7 @@ struct musb_regs { struct musb_epN_regs epN; } ep[16 ]; -} __attribute__((packed, aligned(32))); +} __attribute__((packed, aligned(USB_DMA_MINALIGN)));#endif /*
include/usb.h +10 -0
@@ -29,6 +29,16 @@#include <usb_defs.h> #include <usbdescriptors.h> +/* +* The EHCI spec says that we must align to at least 32 bytes. However, +* some platforms require larger alignment. +*/ +#if ARCH_DMA_MINALIGN >32 +#define USB_DMA_MINALIGN ARCH_DMA_MINALIGN +#else +#define USB_DMA_MINALIGN 32 +#endif +/* Everything is aribtrary */ #define USB_ALTSETTINGALLOC 4#define USB_MAXALTSETTING 128/* Hard limit */
Signature · 1 line
1.7.9.5
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/mcx.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/configs/mcx.h +1 -1
@@ -110,9 +110,9 @@#define CONFIG_OMAP3_GPIO_5 #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_ULPI #define CONFIG_USB_ULPI_VIEWPORT_OMAP -/*#define CONFIG_EHCI_DCACHE*/ /* leave it disabled for now */ #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 154#define CONFIG_OMAP_EHCI_PHY2_RESET_GPIO 152#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
Signature · 1 line
1.7.9.5
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Hi,
Quoted 2 lines · Tom Rini
USB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 6 lines
Hi,
Quoted 2 lines · Tom Rini
USB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
Quoted 9 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.
We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Quoted 2 lines
Regards, Ilya.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Marek,
Quoted 11 lines · Marek Vasut
Quoted 9 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.
BTW, I think this was under #ifdef CONFIG_EHCI_DCACHE last time looked at it. Was this changed by your commit? I think that's the source of the problem this series tries to address: you've taken buggy code out of #ifdef ;) I think it's better to just put it back until upper layers won't be fixed.
Regards, Ilya.
[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/114235
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 13 lines
Dear Marek,
Quoted 10 lines · Marek Vasut
Quoted 9 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.
Quoted 7 lines
Quoted 2 lines
We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.
Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
Quoted 9 lines
BTW, I think this was under #ifdef
CONFIG_EHCI_DCACHElast time looked at it. Was this changed by your commit? I think that's the source of the problem this series tries to address: you've taken buggy code out of #ifdef ;) I think it's better to just put it back until upper layers won't be fixed.Regards, Ilya.
[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/114235
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Marek,
Quoted 5 lines · Marek Vasut
Quoted 4 lines
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a
Surely. (but that probably was an AM3517 with 64 byte cache line)
Quoted 2 lines
patch) and loading from ext2 and vfat (worked).
This is just a coincedence ;)
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 8 lines
Dear Marek,
Quoted 5 lines · Marek Vasut
Quoted 4 lines
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.
Quoted 4 lines
Quoted 2 lines
Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a
Surely. (but that probably was an AM3517 with 64 byte cache line)
m28 is imx28 with 32byte cacheline
Quoted 3 lines
Quoted 1 line
patch) and loading from ext2 and vfat (worked).
This is just a coincedence ;)
Not really, did you check mainline uboot and the fixes in those?
Quoted 1 line
Regards, Ilya.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Marek,
Quoted 2 lines · Marek Vasut
Quoted 1 line
Surely. (but that probably was an AM3517 with 64 byte cache line)
m28 is imx28 with 32byte cacheline
You are lucky then. But some systems have bigger cacheline, right?
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
patch) and loading from ext2 and vfat (worked).
This is just a coincedence ;)
Not really, did you check mainline uboot and the fixes in those?
Surely I did. Let's take a look:
Quoted 8 lines
do {
/* Invalidate dcache */
invalidate_dcache_range((uint32_t)&qh_list,
(uint32_t)&qh_list + sizeof(struct QH));
invalidate_dcache_range((uint32_t)&qh,
(uint32_t)&qh + sizeof(struct QH));
invalidate_dcache_range((uint32_t)qtd,
(uint32_t)qtd + sizeof(qtd));These guys are 32-byte aligned, right? So the assumption is that cacheline is not greater than 32. Sounds like a bug to me (though could be fixed rather easily with USB_MINALIGN introduced by Tom).
Quoted 9 lines
token = hc32_to_cpu(vtd->qt_token);
if (!(token & 0x80))
break;
WATCHDOG_RESET();
} while (get_timer(ts) < timeout);
/* Invalidate the memory area occupied by buffer */
invalidate_dcache_range(((uint32_t)buffer & ~31),
((uint32_t)buffer & ~31) + roundup(length, 32));That's worse. First of all, rounding is wrong: consider buffer=31, length=32. But that's easy to fix too. The bad part is that with writeback cache you can't just enlarge the range to cover the buffer and fit alignment requirements -- this can potentially destroy some changes that are in the same cache line but not yet stored to RAM.
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 8 lines
Dear Marek,
Quoted 3 lines · Marek Vasut
Quoted 1 line
Surely. (but that probably was an AM3517 with 64 byte cache line)
m28 is imx28 with 32byte cacheline
You are lucky then. But some systems have bigger cacheline, right?
Yes, and I just got a perfect system for the job.
Quoted 19 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
patch) and loading from ext2 and vfat (worked).
This is just a coincedence ;)
Not really, did you check mainline uboot and the fixes in those?
Surely I did. Let's take a look:
Quoted 11 lines
do { /* Invalidate dcache */ invalidate_dcache_range((uint32_t)&qh_list, (uint32_t)&qh_list + sizeof(struct QH)); invalidate_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct QH));
Quoted 7 lines
Quoted 3 lines
invalidate_dcache_range((uint32_t)qtd, (uint32_t)qtd + sizeof(qtd));These guys are 32-byte aligned, right? So the assumption is that cacheline is not greater than 32. Sounds like a bug to me (though could be fixed rather easily with
USB_MINALIGNintroduced by Tom).
Oooh, good catch indeed. Actually, look at the structures, even they have some weird alignment crap in them. Maybe that can also be dropped and the alignment shall be fixed properly. I'll have to research this more.
Quoted 13 lines
Quoted 13 lines
token = hc32_to_cpu(vtd->qt_token); if (!(token & 0x80)) break; WATCHDOG_RESET(); } while (get_timer(ts) < timeout); /* Invalidate the memory area occupied by buffer */ invalidate_dcache_range(((uint32_t)buffer & ~31),
Quoted 7 lines
Quoted 1 line
((uint32_t)buffer & ~31) + roundup(length, 32));That's worse. First of all, rounding is wrong: consider buffer=31, length=32. But that's easy to fix too. The bad part is that with writeback cache you can't just enlarge the range to cover the buffer and fit alignment requirements -- this can potentially destroy some changes that are in the same cache line but not yet stored to RAM.
Correct, so we have multiple bugs in here that need to be squashed ASAP.
Ilya, can you possibly submit a patch for this?
Quoted 2 lines
Regards, Ilya.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Marek,
Quoted 13 lines · Marek Vasut
Quoted 13 lines
Quoted 13 lines
do { /* Invalidate dcache */ invalidate_dcache_range((uint32_t)&qh_list, (uint32_t)&qh_list + sizeof(struct QH)); invalidate_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct QH)); invalidate_dcache_range((uint32_t)qtd,
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
(uint32_t)qtd + sizeof(qtd));These guys are 32-byte aligned, right? So the assumption is that cacheline is not greater than 32. Sounds like a bug to me (though could be fixed rather easily with
USB_MINALIGNintroduced by Tom).Oooh, good catch indeed. Actually, look at the structures, even they have some weird alignment crap in them. Maybe that can also be dropped and the alignment shall be fixed properly. I'll have to research this more.
I guess that's because they have to be both address and size aligned.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
token = hc32_to_cpu(vtd->qt_token); if (!(token& 0x80)) break; WATCHDOG_RESET(); } while (get_timer(ts)< timeout); /* Invalidate the memory area occupied by buffer */ invalidate_dcache_range(((uint32_t)buffer& ~31), ((uint32_t)buffer& ~31) + roundup(length, 32));
Quoted 8 lines
Quoted 5 lines
That's worse. First of all, rounding is wrong: consider buffer=31, length=32. But that's easy to fix too. The bad part is that with writeback cache you can't just enlarge the range to cover the buffer and fit alignment requirements -- this can potentially destroy some changes that are in the same cache line but not yet stored to RAM.
Correct, so we have multiple bugs in here that need to be squashed ASAP.
Ilya, can you possibly submit a patch for this?
Hm.. Fixing alignment for qh, qd and stuff is easy, I can surely submit a patch for this. But I don't really know what to do this the last one:
if we are at this point there is no correct way out... We can increase the range to cover the buffer or decrease it to be inside buffer -- but both options are incorrect. Actually we should return error when unaligned buffer is submitted in the first place... But this will likely break a lot of systems... Probably put this check under if (dcache_enabled())?
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 48 lines
Dear Marek,
Quoted 15 lines · Marek Vasut
Quoted 15 lines
Quoted 14 lines
do { /* Invalidate dcache */ invalidate_dcache_range((uint32_t)&qh_list, (uint32_t)&qh_list + sizeof(struct QH)); invalidate_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct QH)); invalidate_dcache_range((uint32_t)qtd, (uint32_t)qtd + sizeof(qtd));Quoted 7 lines
Quoted 3 lines
These guys are 32-byte aligned, right? So the assumption is that cacheline is not greater than 32. Sounds like a bug to me (though could be fixed rather easily with
USB_MINALIGNintroduced by Tom).Oooh, good catch indeed. Actually, look at the structures, even they have some weird alignment crap in them. Maybe that can also be dropped and the alignment shall be fixed properly. I'll have to research this more.
I guess that's because they have to be both address and size aligned.
Quoted 20 lines
Quoted 19 lines
Quoted 5 lines
token = hc32_to_cpu(vtd->qt_token); if (!(token& 0x80)) break;Quoted 8 lines
WATCHDOG_RESET(); } while (get_timer(ts)< timeout); /* Invalidate the memory area occupied by buffer */ invalidate_dcache_range(((uint32_t)buffer& ~31), ((uint32_t)buffer& ~31) + roundup(length, 32));That's worse. First of all, rounding is wrong: consider buffer=31, length=32. But that's easy to fix too. The bad part is that with writeback cache you can't just enlarge the range to cover the buffer and fit alignment requirements -- this can potentially destroy some changes that are in the same cache line but not yet stored to RAM.
Quoted 6 lines
Quoted 3 lines
Correct, so we have multiple bugs in here that need to be squashed ASAP.
Ilya, can you possibly submit a patch for this?
Hm.. Fixing alignment for qh, qd and stuff is easy, I can surely submit a patch for this.
Well ... partial patch is still better than none.
Quoted 7 lines
But I don't really know what to do this the last one:
if we are at this point there is no correct way out... We can increase the range to cover the buffer or decrease it to be inside buffer -- but both options are incorrect. Actually we should return error when unaligned buffer is submitted in the first place... But this will likely break a lot of systems... Probably put this check under if (dcache_enabled())?
Bounce buffer I guess ... with warning that you'll hit performance penalty becaue you're dumb and doing unaligned transfer. And for internal uboot stuff, we can align it.
Quoted 2 lines
Regards, Ilya.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
In reply to Marek VasutQuoted 18 lines · Marek Vasut
Dear Ilya Yanok,
Quoted 16 lines
Dear Marek,
Quoted 12 lines · Marek Vasut
Quoted 9 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Quoted 7 lines
Quoted 4 lines
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Signature · 1 line
Tom
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Tom Rini,
Quoted 24 lines · Marek Vasut
Quoted 23 lines
Dear Ilya Yanok,
Quoted 20 lines
Dear Marek,
Quoted 10 lines · Marek Vasut
Quoted 9 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.Quoted 2 lines
We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.
Quoted 5 lines
Quoted 2 lines
Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Quoted 26 lines · Marek Vasut
Dear Tom Rini,
Quoted 24 lines · Marek Vasut
Quoted 23 lines
Dear Ilya Yanok,
Quoted 20 lines
Dear Marek,
Quoted 10 lines · Marek Vasut
Quoted 9 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.Quoted 2 lines
We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them.
Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for beagle and omap3_evm and leave it on for mcx and see who has time and hardware to fix things for v2012.10.
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120628/d93b888e/attachment.pgp>
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Tom Rini,
Quoted 35 lines · Marek Vasut
Quoted 34 lines
Dear Tom Rini,
Quoted 31 lines · Marek Vasut
Quoted 19 lines
Dear Ilya Yanok,
Quoted 17 lines
Dear Marek,
Quoted 13 lines · Marek Vasut
Quoted 10 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Quoted 8 lines
Quoted 5 lines
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Quoted 6 lines
Quoted 3 lines
Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them.
Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for beagle and
omap3_evm
Didn't you fix the issues?
Quoted 2 lines
and leave it on for mcx and see who has time and
hardware to fix things for v2012.10.
Or we fix it for mcx too until .07 is out ?
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Quoted 44 lines · Marek Vasut
Dear Tom Rini,
Quoted 35 lines · Marek Vasut
Quoted 34 lines
Dear Tom Rini,
Quoted 31 lines · Marek Vasut
Quoted 19 lines
Dear Ilya Yanok,
Quoted 17 lines
Dear Marek,
Quoted 13 lines · Marek Vasut
Quoted 10 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Quoted 8 lines
Quoted 5 lines
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Quoted 6 lines
Quoted 3 lines
Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them.
Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for beagle and
omap3_evm
Quoted 6 lines
Didn't you fix the issues?
Quoted 2 lines
and leave it on for mcx and see who has time and
hardware to fix things for v2012.10.Or we fix it for mcx too until .07 is out ?
To clarify for everyone, the first part of this series fixes some alignment issues for things that were not starting address aligned. There still exist end-address alignment issues within ehci-hcd. The time I have for this problem right now boils down to disable dcache for these boards so that USB is still functional.
Signature · 1 line
Tom
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Tom Rini,
Quoted 50 lines · Marek Vasut
Quoted 49 lines
Dear Tom Rini,
Quoted 35 lines · Marek Vasut
Quoted 34 lines
Dear Tom Rini,
Quoted 31 lines · Marek Vasut
Quoted 19 lines
Dear Ilya Yanok,
Quoted 17 lines
Dear Marek,
Quoted 13 lines · Marek Vasut
Quoted 10 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal.
ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Quoted 8 lines
Quoted 5 lines
Really? Don't forget my old patch [1] then ;)
Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Quoted 6 lines
Quoted 3 lines
Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them.
Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for beagle and
omap3_evmDidn't you fix the issues?
Quoted 2 lines
and leave it on for mcx and see who has time and
hardware to fix things for v2012.10.
Quoted 7 lines
Quoted 1 line
Or we fix it for mcx too until .07 is out ?
To clarify for everyone, the first part of this series fixes some alignment issues for things that were not starting address aligned. There still exist end-address alignment issues within ehci-hcd. The time I have for this problem right now boils down to disable dcache for these boards so that USB is still functional.
To clarify it even further -- it always worked just by sheer coincidence ...
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 67 lines · Marek Vasut
Dear Tom Rini,
Quoted 56 lines · Marek Vasut
Quoted 55 lines
Dear Tom Rini,
Quoted 41 lines · Marek Vasut
Quoted 40 lines
Dear Tom Rini,
Quoted 37 lines · Marek Vasut
Quoted 29 lines
Dear Ilya Yanok,
Quoted 21 lines
Dear Marek,
Quoted 14 lines · Marek Vasut
Quoted 5 lines
Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal. ehci-hcd cache handling is broken
anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don'tQuoted 8 lines
care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver.
Quoted 3 lines
We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there.
Quoted 5 lines
Really? Don't forget my old patch [1] then ;) Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled.
Quoted 3 lines
Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked).
So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Quoted 6 lines
Quoted 3 lines
Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them.
Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for beagle and
omap3_evmDidn't you fix the issues?
Quoted 2 lines
and leave it on for mcx and see who has time and hardware to fix things for v2012.10.
Quoted 8 lines
Quoted 1 line
Or we fix it for mcx too until .07 is out ?
To clarify for everyone, the first part of this series fixes some alignment issues for things that were not starting address aligned. There still exist end-address alignment issues within ehci-hcd. The time I have for this problem right now boils down to disable dcache for these boards so that USB is still functional.
Quoted 2 lines
To clarify it even further -- it always worked just by sheer coincidence ...
No, it didn't. It used to work in a timely manner, with dcache disabled but support enabled at build time. Now it works, in an unusably slow manner, with dcache disabled but support enabled at build time. It continues to work in a timely manner with dcache support disabled at build time. On any platform with >32byte alignment requirements for cache flushing.
- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQIcBAEBAgAGBQJP7Q+BAAoJENk4IS6UOR1WhRIP/1NlLlF4iqiPAr7ZhnVWHUS5 IO76xwcIc69ZZsbD46EAG3XvUpmSkszP7fmr9KZerPYylH4Ne4TOtxqF7yg8rVnx 856tosee9PU4l5yUbt3JbT/XYoT5ivcw7n058PO3BliHcSSZ7BEMsaKOEOUzm+x7 Alzu76UM+YUkIGfGnQMWQnAcT0alNp/aa2KZriWPCOTKj2NKghRuO7xlBF6KGSBz S/9MaiWnC0PvkCc2fhYt7JsIItz19gnx31M/JhU6gTxOR1WfMAlsQfjDXs5wTZI+ rCuW0H/GX9O3FpXG33Uecf2dzg8e7t45qXRND/7sfHCx4M2nE4HWY+RvSP1XCacH MTTqeBZm7RpDWKO7P4dQW8GWLkUzNYLOySTyLp2Rnh7882C1QaNbRWplvC+4NB3g KbSK8H0PqEu/pVJ3Dl2kutMY6PcKX/H1l7x4swEIoNtor6Lx3wE5rzhjjDqElGl7 HVWVjI2Hgco9F043pCjkS3Mb7pNIcJ3/yMjDC8C7PWDLrW9+fPwkbZxqCTJGaBWS Qlo6RbKxE4yDwDtbJXJspy1ML0jK2DO0NbFD+LDfIrecpKUpTtxBee/b4E6DGcil LJ6OWprUnxYVM4LHUjdvI9pcX92gwOw6EXpcvPNiHOl9ZO8nThCs5QElg2OAJmlw qgNzEI3zqq4GimdS+0oh
=Lc3K
-----END PGP SIGNATURE-----
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
In reply to Marek VasutDear Marek,
Quoted 6 lines · Marek Vasut
Quoted 5 lines
To clarify for everyone, the first part of this series fixes some alignment issues for things that were not starting address aligned. There still exist end-address alignment issues within ehci-hcd. The time I have for this problem right now boils down to disable dcache for these boards so that USB is still functional.
To clarify it even further -- it always worked just by sheer coincidence ...
Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 9 lines
Dear Marek,
Quoted 6 lines · Marek Vasut
Quoted 5 lines
To clarify for everyone, the first part of this series fixes some alignment issues for things that were not starting address aligned. There still exist end-address alignment issues within ehci-hcd. The time I have for this problem right now boils down to disable dcache for these boards so that USB is still functional.
Quoted 6 lines
Quoted 2 lines
To clarify it even further -- it always worked just by sheer coincidence
...Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
I see what you're after. But do you consider runtime disabling the cache is the way to go or it's a way of hiding bugs?
Quoted 1 line
Regards, Ilya.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Marek,
Quoted 5 lines · Marek Vasut
Quoted 3 lines
Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
I see what you're after. But do you consider runtime disabling the cache is the way to go or it's a way of hiding bugs?
Both ;) And now we are going to hide even more bugs with compile-time disabling :(
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Quoted 6 lines · Ilya Yanok
Dear Marek,
Quoted 3 lines · Marek Vasut
Quoted 3 lines
Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
Quoted 6 lines
Quoted 3 lines
I see what you're after. But do you consider runtime disabling the cache is the
way to go or it's a way of hiding bugs?Both ;) And now we are going to hide even more bugs with compile-time disabling :(
Does someone wish to argue we should disable USB support instead on these platforms? I don't see anyone arguing "I have time to fix this for v2012.07".
Signature · 1 line
Tom
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Hi Tom,
Quoted 7 lines · Tom Rini
Quoted 6 lines · Ilya Yanok
Dear Marek,
Quoted 3 lines · Marek Vasut
Quoted 3 lines
Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
I see what you're after. But do you consider runtime disabling the cache is the
way to go or it's a way of hiding bugs?Both ;) And now we are going to hide even more bugs with compile-time disabling :(
Does someone wish to argue we should disable USB support instead on these platforms? I don't see anyone arguing "I have time to fix this for v2012.07".
I just looked at the code more carefully and it seems that most of the upper layers are in much better shape than I thought. So I think we should just extend your 2/6 patch to fix both address and size for structs QH and qtd and don't mess with buffer at all: if we got unaligned buffer -- it's definetely upper layer bug so we should produce some noise in this case. As I said upper layers seems to be in good shape so hopefully there won't be too much noise.
Hm, probably we should put buffer invalidation under if(dcache_enabled()) to leave run-time cache disabling as rescue option for broken upper-layer code..
I'm working on the patch now and hopefully will post it this night.
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Dear Ilya Yanok,
Quoted 31 lines
Hi Tom,
Quoted 13 lines · Tom Rini
Quoted 12 lines · Ilya Yanok
Dear Marek,
Quoted 8 lines · Marek Vasut
Quoted 4 lines
Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
I see what you're after. But do you consider runtime disabling the cache is the
way to go or it's a way of hiding bugs?Quoted 6 lines
Quoted 2 lines
Both ;) And now we are going to hide even more bugs with compile-time disabling :(
Does someone wish to argue we should disable USB support instead on these platforms? I don't see anyone arguing "I have time to fix this for v2012.07".
I just looked at the code more carefully and it seems that most of the upper layers are in much better shape than I thought. So I think we should just extend your 2/6 patch to fix both address and size for structs QH and qtd and don't mess with buffer at all: if we got unaligned buffer -- it's definetely upper layer bug so we should produce some noise in this case. As I said upper layers seems to be in good shape so hopefully there won't be too much noise.
Quoted 5 lines
Hm, probably we should put buffer invalidation under if(dcache_enabled()) to leave run-time cache disabling as rescue option for broken upper-layer code..
I'm working on the patch now and hopefully will post it this night.
Ilya, thank you for saving my back ;-)
And thank you for investing your time into this.
Quoted 1 line
Regards, Ilya.
Best regards,
Marek Vasut
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
Hi All,
Quoted 3 lines · Marek Vasut
Ilya, thank you for saving my back ;-)
And thank you for investing your time into this.
You are welcome ;)
Just posted the patch. No dealing with unaligned buffers from upper layers for now but at least fatload with aligned address works fine.
Regards, Ilya.
PATCH v4 3/6 mcx: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniDear Tom Rini,
Quoted 12 lines · Ilya Yanok
Quoted 11 lines
Dear Marek,
Quoted 7 lines · Marek Vasut
Quoted 3 lines
Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time.
I see what you're after. But do you consider runtime disabling the cache is the
way to go or it's a way of hiding bugs?
Quoted 6 lines
Quoted 2 lines
Both ;) And now we are going to hide even more bugs with compile-time disabling :(
Does someone wish to argue we should disable USB support instead on these platforms? I don't see anyone arguing "I have time to fix this for v2012.07".
I don't have time to fix this, I'm hacking 200% already.
Best regards,
Marek Vasut
PATCH v4 4/6 omap3_beagle: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/omap3_beagle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/configs/omap3_beagle.h +1 -1
@@ -131,7 +131,7 @@#define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP -/*#define CONFIG_EHCI_DCACHE*/ /* leave it disabled for now */ +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 147#define CONFIG_USB_ULPI
Signature · 1 line
1.7.9.5
PATCH v4 5/6 omap4_panda: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/omap4_panda.h | 1 + 1 file changed, 1 insertion(+)
include/configs/omap4_panda.h +1 -0
@@ -38,6 +38,7 @@#define CONFIG_USB_HOST #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_STORAGE #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
Signature · 1 line
1.7.9.5
PATCH v4 5/6 omap4_panda: Disable DCACHE since USB EHCI is enabled
Hi Tom,
[snip..]
Quoted 4 lines
#define
CONFIG_USB_HOST
#defineCONFIG_USB_EHCI
#defineCONFIG_USB_EHCI_OMAP
+#defineCONFIG_SYS_DCACHE_OFF/*USB_EHCIis unusable with
DCACHE
Quoted 3 lines
support */
#defineCONFIG_USB_STORAGE
#defineCONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS3
While, this looks like to be the safest option till we fix both the cache issue and alignment issue in USB stack.
Thanks,
Sricharan
R
PATCH v4 5/6 omap4_panda: Disable DCACHE since USB EHCI is enabled
Hi,
Quoted 6 lines · Sricharan R
Hi Tom,
[snip..]Quoted 4 lines
?#define
CONFIG_USB_HOST
?#defineCONFIG_USB_EHCI
?#defineCONFIG_USB_EHCI_OMAP
+#defineCONFIG_SYS_DCACHE_OFF? ? ? ?/*USB_EHCIis unusable with
Quoted 8 lines
DCACHE
Quoted 3 lines
support */
?#defineCONFIG_USB_STORAGE
?#defineCONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS3?While, this looks like to be the safest option till ?we fix both the cache issue and alignment issue in USB stack.
Thanks,
Sricharan
PATCH v4 6/6 tam3517-common: Disable DCACHE since USB EHCI is enabled
In reply to Tom RiniUSB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use.
--- include/configs/tam3517-common.h | 1 + 1 file changed, 1 insertion(+)
include/configs/tam3517-common.h +1 -0
@@ -100,6 +100,7 @@#define CONFIG_OMAP3_GPIO_5 #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_ULPI #define CONFIG_USB_ULPI_VIEWPORT_OMAP #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 25
Signature · 1 line
1.7.9.5
cover USB and cache related fixes
In reply to Tom RiniDear Tom Rini,
Quoted 5 lines
Hey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32).
Good thing, it made a bug surface ;-)
Quoted 3 lines
First, the
dcache_offcall that
ehci-omap had been doing was now not happening soon enough to paper over the cache issues.
Hm, is the dcache_off() call implemented properly so nothing is lost when you shut off the cache btw?
Quoted 11 lines
This call is removed in patch 1. Second, when we have
dcache support compiled in but turned off via 'dcache off' the cache routines spam the console about alignment issues when a cache flush is attempted. This is a problem in that it makes operations extremely slow (as we're spending all our time spitting messages to console). The second patch makes the flush routines return when the dcache is off. The last two patches deal with the same problem, for EHCI and for MUSB. The USB spec says that 32 bytes is the minimum alignment but we need larger alignment when the cache is larger. Note that we can't useMAX()here as gcc doesn't allow that expansion inside of align(..).
Quoted 2 lines
Tested on
omap3_beagle(which was previously broken) and a MAKEALL -a arm looks good too.
Good job Tom, thanks for spending time on fixing this!
Best regards,
Marek Vasut
cover USB and cache related fixes
Quoted 8 lines · Marek Vasut
Dear Tom Rini,
Quoted 5 lines
Hey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32).
Quoted 8 lines
Good thing, it made a bug surface ;-)
Quoted 3 lines
First, the
dcache_offcall that
ehci-omap had been doing was now not happening soon enough to paper over the cache issues.Hm, is the
dcache_off()call implemented properly so nothing is lost when you shut off the cache btw?
As best I can tell, yes. It will do a dcache_flush_all() and then set the correct bits.
Signature · 1 line
Tom
cover USB and cache related fixes
Dear Tom Rini,
Quoted 15 lines · Marek Vasut
Quoted 14 lines
Dear Tom Rini,
Quoted 5 lines
Hey all,
In commit b8adb12 the cache flushing behavior was changed for the EHCI stack. This change showed a few different problems on TI platforms (where our cacheline size is 64 not 32).
Good thing, it made a bug surface ;-)
Quoted 3 lines
First, the
dcache_offcall that
ehci-omap had been doing was now not happening soon enough to paper over the cache issues.
Quoted 5 lines
Quoted 2 lines
Hm, is the
dcache_off()call implemented properly so nothing is lost when you shut off the cache btw?As best I can tell, yes. It will do a
dcache_flush_all()and then set the correct bits.
Ok, good
Best regards,
Marek Vasut