[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Eddie Cai]

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 <eddie.cai.linux@gmail.com>
---
 drivers/usb/host/dwc2.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 5ac602e..a151ba6 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -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);
-- 
2.10.2

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Brüns, Stefan]

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 <eddie.cai.linux@gmail.com>

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 <stefan.bruens@rwth-aachen.de>

to your v2.

Kind regards,

Stefan

> ---
>  drivers/usb/host/dwc2.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index 5ac602e..a151ba6 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -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);

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/03/2017 04:43 PM, Brüns, Stefan wrote:
> 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 <eddie.cai.linux@gmail.com>
> 
> 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 <stefan.bruens@rwth-aachen.de>
> 
> to your v2.

Works for me too, looking forward to v2.

-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[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.


Thanks,
- Kever
On 04/01/2017 02:51 PM, 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 <eddie.cai.linux@gmail.com>
> ---
>   drivers/usb/host/dwc2.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index 5ac602e..a151ba6 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -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);

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

Hi,

On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
> 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

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/05/2017 04:21 AM, Simon Glass wrote:
> Hi,
> 
> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>> 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/
> 

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

-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

Hi Marek,

On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
> On 04/05/2017 04:21 AM, Simon Glass wrote:
>> Hi,
>>
>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>> 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/
>>
>
> 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

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/05/2017 12:08 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>> Hi,
>>>
>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>> 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/
>>>
>>
>> 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.

> 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.

-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

+Tom

Hi Marek,

On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
> On 04/05/2017 12:08 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>> 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/
>>>>
>>>
>>> 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.
>
>> 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

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/05/2017 05:03 PM, Simon Glass wrote:
> +Tom
> 
> Hi Marek,
> 
> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>> 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/
>>>>>
>>>>
>>>> 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.
>>
>>> 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!

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.

> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

Hi Marek,

On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
> On 04/05/2017 05:03 PM, Simon Glass wrote:
>> +Tom
>>
>> Hi Marek,
>>
>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>> 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/
>>>>>>
>>>>>
>>>>> 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.
>>>
>>>> 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!
>
> 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 :-)

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

OK.

Regards,
Simon

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/06/2017 03:24 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
>> On 04/05/2017 05:03 PM, Simon Glass wrote:
>>> +Tom
>>>
>>> Hi Marek,
>>>
>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>>> 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/
>>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>> 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!
>>
>> 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 ...

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


-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

Hi Marek,

On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote:
> On 04/06/2017 03:24 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
>>> On 04/05/2017 05:03 PM, Simon Glass wrote:
>>>> +Tom
>>>>
>>>> Hi Marek,
>>>>
>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>>>> 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/
>>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>> 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!
>>>
>>> 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 ...

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

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

Regards,
Simon

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/06/2017 04:40 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote:
>> On 04/06/2017 03:24 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
>>>> On 04/05/2017 05:03 PM, Simon Glass wrote:
>>>>> +Tom
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>>>>> 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/
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>>> 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!
>>>>
>>>> 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 ...
> 
> 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.

-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

Hi Marek,

On 6 April 2017 at 04:22, Marek Vasut <marex@denx.de> wrote:
> On 04/06/2017 04:40 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote:
>>> On 04/06/2017 03:24 AM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
>>>>> On 04/05/2017 05:03 PM, Simon Glass wrote:
>>>>>> +Tom
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>>>>>> 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/
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>>> 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!
>>>>>
>>>>> 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 ...
>>
>> 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

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Marek Vasut]

On 04/06/2017 04:06 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 6 April 2017 at 04:22, Marek Vasut <marex@denx.de> wrote:
>> On 04/06/2017 04:40 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote:
>>>> On 04/06/2017 03:24 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 04/05/2017 05:03 PM, Simon Glass wrote:
>>>>>>> +Tom
>>>>>>>
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>>>>>>> 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/
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>
>>>>>>>>> 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!
>>>>>>
>>>>>> 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 ...
>>>
>>> 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.

> 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 .

-- 
Best regards,
Marek Vasut

[U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Simon Glass]

Hi Marek,

On 6 April 2017 at 09:33, Marek Vasut <marex@denx.de> wrote:
> On 04/06/2017 04:06 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 6 April 2017 at 04:22, Marek Vasut <marex@denx.de> wrote:
>>> On 04/06/2017 04:40 AM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote:
>>>>> On 04/06/2017 03:24 AM, Simon Glass wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 04/05/2017 05:03 PM, Simon Glass wrote:
>>>>>>>> +Tom
>>>>>>>>
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>>>>>>>>>> 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/
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>>> 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!
>>>>>>>
>>>>>>> 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 ...
>>>>
>>>> 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.

>
>> 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