usb: dwc2: invalidate the dcache before starting the DMA

U-Boot Eddie Cai 5 participants 17 messages Apr 1, 2017 – Apr 6, 2017 USB

Share & tools

Eddie Cai

author

2017-04-01 06:51 UTC #0 lore raw

We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>

---
 drivers/usb/host/dwc2.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

drivers/usb/host/dwc2.c +11 -6

index 5ac602e..a151ba6 100644
@@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer,
 	       (*pid << DWC2_HCTSIZ_PID_OFFSET),
 	       &hc_regs->hctsiz);
 
-	if (!in && xfer_len) {
-		memcpy(aligned_buffer, buffer, xfer_len);
-
-		flush_dcache_range((unsigned long)aligned_buffer,
-				   (unsigned long)aligned_buffer +
-				   roundup(xfer_len, ARCH_DMA_MINALIGN));
+	if (xfer_len) {
+		if(in){
+			invalidate_dcache_range((unsigned long)aligned_buffer,
+					   (unsigned long)aligned_buffer +
+					   roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}else{
+			memcpy(aligned_buffer, buffer, xfer_len);
+			flush_dcache_range((unsigned long)aligned_buffer,
+					   (unsigned long)aligned_buffer +
+					   roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}
 	}
 
 	writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);

Signature · 1 line

2.10.2

Brüns, Stefan

Reviewed-by

2017-04-03 14:43 UTC #1 lore raw

Quoted 7 lines · On Samstag, 1. April 2017 08:51:39 CEST Eddie Cai wrote:

We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on.
Signed-off-by Eddie Cai <[email protected]>

Can you please run the patch through checkpatch, I can at least spot some missing whitespace.

You can add my

Reviewed-by Stefan BrÃ¼ns <[email protected]>

to your v2.

Kind regards,

Stefan

Quoted 32 lines

---
 drivers/usb/host/dwc2.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

drivers/usb/host/dwc2.c +11 -6

index 5ac602e..a151ba6 100644
@@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs
 *hc_regs, void *aligned_buffer, (*pid << DWC2_HCTSIZ_PID_OFFSET),
 	       &hc_regs->hctsiz);
 
-	if (!in && xfer_len) {
-		memcpy(aligned_buffer, buffer, xfer_len);
-
-		flush_dcache_range((unsigned long)aligned_buffer,
-				   (unsigned long)aligned_buffer +
-				   roundup(xfer_len, ARCH_DMA_MINALIGN));
+	if (xfer_len) {
+		if(in){
+			invalidate_dcache_range((unsigned long)aligned_buffer,
+					   (unsigned long)aligned_buffer +
+					   roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}else{
+			memcpy(aligned_buffer, buffer, xfer_len);
+			flush_dcache_range((unsigned long)aligned_buffer,
+					   (unsigned long)aligned_buffer +
+					   roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}
 	}
 
 	writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);

Marek Vasut

2017-04-03 19:33 UTC #2 lore raw

Quoted 9 lines · Stefan

Quoted 7 lines · On Samstag, 1. April 2017 08:51:39 CEST Eddie Cai wrote:
We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on.
Signed-off-by Eddie Cai <[email protected]>

Quoted 8 lines

Can you please run the patch through checkpatch, I can at least spot some missing whitespace.
You can add my
Reviewed-by Stefan Brüns <[email protected]>
to your v2.

Works for me too, looking forward to v2.

Signature · 2 lines

Best regards,
Marek Vasut

Kever Yang

2017-04-05 01:26 UTC #3 lore raw

In reply to Eddie Cai

Hi Eddie,

We should only need to do only one time cache operation for a buffer

ready to do DMA transfer, so you need to remove another cache invalidate

operation for the same buffer in the same function.

Thanks,
- Kever

Quoted 39 lines · Eddie Cai

Signed-off-by Eddie Cai <[email protected]>

---
  drivers/usb/host/dwc2.c | 17 +++++++++++------
  1 file changed, 11 insertions(+), 6 deletions(-)

drivers/usb/host/dwc2.c +11 -6

index 5ac602e..a151ba6 100644
@@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer,
  	       (*pid << DWC2_HCTSIZ_PID_OFFSET),
  	       &hc_regs->hctsiz);
 
-	if (!in && xfer_len) {
-		memcpy(aligned_buffer, buffer, xfer_len);
-
-		flush_dcache_range((unsigned long)aligned_buffer,
-				   (unsigned long)aligned_buffer +
-				   roundup(xfer_len, ARCH_DMA_MINALIGN));
+	if (xfer_len) {
+		if(in){
+			invalidate_dcache_range((unsigned long)aligned_buffer,
+					   (unsigned long)aligned_buffer +
+					   roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}else{
+			memcpy(aligned_buffer, buffer, xfer_len);
+			flush_dcache_range((unsigned long)aligned_buffer,
+					   (unsigned long)aligned_buffer +
+					   roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}
  	}
 
  	writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);

Simon Glass

2017-04-05 02:21 UTC #4 lore raw

Hi,

Quoted 8 lines · Kever Yang

Hi Eddie,
We should only need to do only one time cache operation for a buffer
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.

I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:

http://patchwork.ozlabs.org/patch/746917/

Regards,
Simon

Marek Vasut

2017-04-05 09:35 UTC #5 lore raw

Quoted 8 lines · Simon Glass

Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer

Quoted 9 lines

Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/

This feels like papering over a problem though ... which will bite you later anyway.

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2017-04-05 10:08 UTC #6 lore raw

Hi Marek,

Quoted 13 lines · Marek Vasut

Quoted 12 lines · Simon Glass
Hi,
Quoted 8 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.

Quoted 8 lines

Quoted 5 lines
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.

I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.

Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.

Regards,
Simon

Marek Vasut

2017-04-05 10:21 UTC #7 lore raw

Quoted 22 lines · Simon Glass

Hi Marek,
Quoted 19 lines · Marek Vasut
Quoted 17 lines · Simon Glass
Hi,
Quoted 8 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/

Quoted 6 lines

Quoted 2 lines
This feels like papering over a problem though ... which will bite you later anyway.
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.

And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.

Quoted 3 lines

Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2017-04-05 15:03 UTC #8 lore raw

+Tom

Hi Marek,

Quoted 26 lines · Marek Vasut

Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.

Quoted 10 lines

Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.

I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.

If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.

On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!

Regards,
Simon

Marek Vasut

2017-04-05 21:34 UTC #9 lore raw

Quoted 45 lines · Simon Glass

+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.

Quoted 8 lines

If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!

So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.

If this also needs to be fixed in DM, so be it.

Quoted 3 lines

Regards,
Simon

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2017-04-06 01:24 UTC #10 lore raw

Hi Marek,

Quoted 52 lines · Marek Vasut

Quoted 51 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.

Quoted 6 lines

Quoted 2 lines
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.

Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)

Quoted 2 lines

If this also needs to be fixed in DM, so be it.

OK.

Regards,
Simon

Marek Vasut

2017-04-06 01:32 UTC #11 lore raw

Quoted 58 lines · Simon Glass

Hi Marek,
Quoted 55 lines · Marek Vasut
Quoted 53 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!

Quoted 7 lines

Quoted 3 lines
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.
Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)

I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ...

Quoted 8 lines

Quoted 2 lines
If this also needs to be fixed in DM, so be it.
OK.
Regards,
Simon

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2017-04-06 02:40 UTC #12 lore raw

Hi Marek,

Quoted 63 lines · Marek Vasut

Quoted 62 lines · Simon Glass
Hi Marek,
Quoted 58 lines · Marek Vasut
Quoted 45 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
Quoted 8 lines
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.

Quoted 6 lines

Quoted 3 lines
Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)
I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ...

OK, well up to you. What sort of problems?

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
If this also needs to be fixed in DM, so be it.
OK.
Regards,
Simon

Regards,
Simon

Marek Vasut

2017-04-06 10:22 UTC #13 lore raw

Quoted 66 lines · Simon Glass

Hi Marek,
Quoted 63 lines · Marek Vasut
Quoted 62 lines · Simon Glass
Hi Marek,
Quoted 58 lines · Marek Vasut
Quoted 45 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
Quoted 8 lines
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)
I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ...
OK, well up to you. What sort of problems?

Some sticks are not detected for me and adding a small delay here magically fixes it. I suspect I'm hitting this cache issue.

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2017-04-06 14:06 UTC #14 lore raw

Hi Marek,

Quoted 71 lines · Marek Vasut

Quoted 70 lines · Simon Glass
Hi Marek,
Quoted 67 lines · Marek Vasut
Quoted 65 lines · Simon Glass
Hi Marek,
Quoted 52 lines · Marek Vasut
Quoted 51 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
Quoted 6 lines
Quoted 2 lines
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.
Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ...
OK, well up to you. What sort of problems?
Some sticks are not detected for me and adding a small delay here magically fixes it. I suspect I'm hitting this cache issue.

Is this with CONFIG_DM_USB or without?

Also does your platform have a write-through or write-back cache?

Regards,
Simon

Marek Vasut

2017-04-06 15:33 UTC #15 lore raw

Quoted 77 lines · Simon Glass

Hi Marek,
Quoted 74 lines · Marek Vasut
Quoted 73 lines · Simon Glass
Hi Marek,
Quoted 69 lines · Marek Vasut
Quoted 58 lines · Simon Glass
Hi Marek,
Quoted 55 lines · Marek Vasut
Quoted 53 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!
Quoted 7 lines
Quoted 3 lines
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.
Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)
I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ...

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
OK, well up to you. What sort of problems?
Some sticks are not detected for me and adding a small delay here magically fixes it. I suspect I'm hitting this cache issue.
Is this with CONFIG_DM_USB or without?

With.

Quoted 1 line

Also does your platform have a write-through or write-back cache?

I think WT, but I'm not 100% sure . I can check when I have access to the board . It's SoCFPGA, C-A9 .

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2017-04-06 15:57 UTC #16 lore raw

Hi Marek,

Quoted 78 lines · Marek Vasut

Quoted 77 lines · Simon Glass
Hi Marek,
Quoted 74 lines · Marek Vasut
Quoted 73 lines · Simon Glass
Hi Marek,
Quoted 69 lines · Marek Vasut
Quoted 58 lines · Simon Glass
Hi Marek,
Quoted 55 lines · Marek Vasut
Quoted 53 lines · Simon Glass
+Tom
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Glass
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 8 lines · Simon Glass
Hi,
Quoted 5 lines · Kever Yang
Hi Eddie,
We should only need to do only one time cache operation for a buffer
Quoted 9 lines
Quoted 3 lines
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
I think this is a more general problem and might cause issues with other drivers. So I have sent this patch:
http://patchwork.ozlabs.org/patch/746917/
This feels like papering over a problem though ... which will bite you later anyway.
Quoted 10 lines
Quoted 3 lines
I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on.
And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly.
Quoted 3 lines
Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off.
I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing.
If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM.
On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM!
Quoted 7 lines
Quoted 3 lines
So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that.
Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-)
I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ...

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
OK, well up to you. What sort of problems?
Some sticks are not detected for me and adding a small delay here magically fixes it. I suspect I'm hitting this cache issue.
Is this with CONFIG_DM_USB or without?
With.

OK, then this is no different from the rpi problem. My DM fix should fix it for you. The current driver (without DM) works fine.

Quoted 5 lines

Quoted 2 lines
Also does your platform have a write-through or write-back cache?
I think WT, but I'm not 100% sure . I can check when I have access to the board . It's SoCFPGA, C-A9 .

I ask because with a WT cache the problem fixes itself after a short time (i.e. if you wait a bit before kicking the DMA the cache will have had time to flush).

Regards,
Simon