dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

PATCH New Faiz Abbas 5 participants 24 messages Sep 19, 2017 – Oct 11, 2017

Subsystems (2)

USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Faiz Abbas

author

2017-09-19 11:15 UTC #0 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

A flush of the cache is required before any DMA access can take place. The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.

Thus, allocate memory for ep0_trb in multiples of cache line size.

Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.

Therefore, flush cache using ep0_trb_addr which is always cache aligned.

Signed-off-by Faiz Abbas <[email protected]>

---
 drivers/usb/dwc3/ep0.c    | 7 ++++---
 drivers/usb/dwc3/gadget.c | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

drivers/usb/dwc3/ep0.c +4 -3

index e61d980..f3a17a1 100644
@@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
 				| DWC3_TRB_CTRL_LST);
 
 	dwc3_flush_cache((uintptr_t)buf_dma, len);
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	if (chain)
 		return 0;

@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 	if (!r)
 		return;
 
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
 	if (status == DWC3_TRBSTS_SETUP_PENDING) {

@@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 			ur->actual += transferred;
 
 			trb++;
-			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
+					 sizeof(*trb) * 2);
 			length = trb->size & DWC3_TRB_SIZE_MASK;
 
 			ep0->free_slot = 0;

drivers/usb/dwc3/gadget.c +2 -1

index e065c5a..895a5bc 100644
@@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err0;
 	}
 
-	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
+	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
+						CACHELINE_SIZE),
 					  (unsigned long *)&dwc->ep0_trb_addr);
 	if (!dwc->ep0_trb) {
 		dev_err(dwc->dev, "failed to allocate ep0 trb\n");

Signature · 1 line

2.7.4

Faiz Abbas

author

2017-10-03 07:35 UTC #1 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 7 lines · Faiz Abbas

A flush of the cache is required before any DMA access can take place. The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.

Quoted 7 lines

Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>

Gentle ping.

Thanks,
Faiz

Andy Shevchenko

2017-10-03 09:08 UTC #2 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 18 lines · Faiz Abbas

Hi,
Quoted 15 lines · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
The minimum size that can be flushed from the cache is one cache line
size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.
Also, when local variable trb is assigned to dwc->ep0_trb[1] and used
to flush cache, it leads to cache misaligned messages as only the base
address dwc->ep0_trb is cache aligned.

Quoted 6 lines

Quoted 4 lines
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>
Gentle ping.

Can you resend with Felipe Balbi included?

And I'm not sure Vincent is anyhow related to this anymore (or even works with us).

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Marek Vasut

2017-10-03 09:43 UTC #3 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 19 lines · Andy Shevchenko

Quoted 18 lines · Faiz Abbas
Hi,
Quoted 15 lines · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
The minimum size that can be flushed from the cache is one cache line
size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.
Also, when local variable trb is assigned to dwc->ep0_trb[1] and used
to flush cache, it leads to cache misaligned messages as only the base
address dwc->ep0_trb is cache aligned.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>
Gentle ping.
Can you resend with Felipe Balbi included?

You can just reply to the original email and add people to CC of that reply instead of resending ?

Quoted 3 lines

And I'm not sure Vincent is anyhow related to this anymore (or even works with us).

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-03 09:58 UTC #4 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

In reply to Faiz Abbas

Hi,

+Felipe Balbi

Quoted 57 lines · Faiz Abbas

A flush of the cache is required before any DMA access can take place. The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.
Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>
---
 drivers/usb/dwc3/ep0.c    | 7 ++++---
 drivers/usb/dwc3/gadget.c | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/usb/dwc3/ep0.c +4 -3
index e61d980..f3a17a1 100644
@@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
 				| DWC3_TRB_CTRL_LST);
 
 	dwc3_flush_cache((uintptr_t)buf_dma, len);
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	if (chain)
 		return 0;
@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 	if (!r)
 		return;
 
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
 	if (status == DWC3_TRBSTS_SETUP_PENDING) {
@@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 			ur->actual += transferred;
 
 			trb++;
-			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
+					 sizeof(*trb) * 2);
 			length = trb->size & DWC3_TRB_SIZE_MASK;
 
 			ep0->free_slot = 0;
drivers/usb/dwc3/gadget.c +0 -0
index e065c5a..895a5bc 100644

drivers/usb/dwc3/gadget.c int dwc3_gadget_init(struct dwc3 *dwc) · line 2574

Quoted 9 lines

drivers/usb/dwc3/gadget.c +2 -1

@@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err0;
 	}
 
-	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
+	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
+						CACHELINE_SIZE),
 					  (unsigned long *)&dwc->ep0_trb_addr);
 	if (!dwc->ep0_trb) {
 		dev_err(dwc->dev, "failed to allocate ep0 trb\n");

Thanks,
Faiz

Marek Vasut

2017-10-03 12:04 UTC #5 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

In reply to Faiz Abbas

Quoted 1 line · Faiz Abbas

A flush of the cache is required before any DMA access can take place.

You mean invalidation for inbound DMA, flush for outbound DMA, right ?

Quoted 32 lines

The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.
Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>
---
 drivers/usb/dwc3/ep0.c    | 7 ++++---
 drivers/usb/dwc3/gadget.c | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/usb/dwc3/ep0.c +1 -1
index e61d980..f3a17a1 100644
@@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
 				| DWC3_TRB_CTRL_LST);
 
 	dwc3_flush_cache((uintptr_t)buf_dma, len);
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	if (chain)
 		return 0;

drivers/usb/dwc3/ep0.c static void dwc3_ep0_complete_data(struct dwc3 *dwc, · line 793

Quoted 5 lines

drivers/usb/dwc3/ep0.c +1 -1

@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 	if (!r)
 		return;
 
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);

Why *2 ?

Quoted 17 lines

drivers/usb/dwc3/ep0.c +2 -1

 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
 	if (status == DWC3_TRBSTS_SETUP_PENDING) {

@@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 			ur->actual += transferred;
 
 			trb++;
-			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
+					 sizeof(*trb) * 2);
 			length = trb->size & DWC3_TRB_SIZE_MASK;
 
 			ep0->free_slot = 0;

drivers/usb/dwc3/gadget.c +0 -0

index e065c5a..895a5bc 100644

drivers/usb/dwc3/gadget.c int dwc3_gadget_init(struct dwc3 *dwc) · line 2571

Quoted 6 lines

drivers/usb/dwc3/gadget.c +2 -1

@@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err0;
 	}
 
-	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
+	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
+						CACHELINE_SIZE),

AFAIU dma_alloc_coherent() should mark the memory area uncachable .

Quoted 4 lines

drivers/usb/dwc3/gadget.c +0 -0

 					  (unsigned long *)&dwc->ep0_trb_addr);
 	if (!dwc->ep0_trb) {
 		dev_err(dwc->dev, "failed to allocate ep0 trb\n");

Signature · 2 lines

Best regards,
Marek Vasut

Dr. Philipp Tomsich

2017-10-03 12:18 UTC #6 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Marek,

Quoted 67 lines · Marek Vasut

Quoted 1 line · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
You mean invalidation for inbound DMA, flush for outbound DMA, right ?
Quoted 32 lines
The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.
Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>
---
drivers/usb/dwc3/ep0.c | 7 ++++---
drivers/usb/dwc3/gadget.c | 3 ++-
2 files changed, 6 insertions(+), 4 deletions(-)
drivers/usb/dwc3/ep0.c +1 -1
index e61d980..f3a17a1 100644
@@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
 				| DWC3_TRB_CTRL_LST);
 
 	dwc3_flush_cache((uintptr_t)buf_dma, len);
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	if (chain)
 		return 0;
drivers/usb/dwc3/ep0.c static void dwc3_ep0_complete_data(struct dwc3 *dwc, · line 793
Quoted 5 lines
drivers/usb/dwc3/ep0.c +1 -1
@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 	if (!r)
 		return;
 
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Quoted 20 lines
drivers/usb/dwc3/ep0.c +2 -1
 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
 	if (status == DWC3_TRBSTS_SETUP_PENDING) {
@@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 			ur->actual += transferred;
 
 			trb++;
-			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
+					 sizeof(*trb) * 2);
 			length = trb->size & DWC3_TRB_SIZE_MASK;
 
 			ep0->free_slot = 0;
drivers/usb/dwc3/gadget.c +0 -0
index e065c5a..895a5bc 100644
@@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err0;
 	}
 

Quoted 5 lines

drivers/usb/dwc3/gadget.c int dwc3_gadget_init(struct dwc3 *dwc) · line 2571
Quoted 3 lines
drivers/usb/dwc3/gadget.c +2 -1
-	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
+	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
+						CACHELINE_SIZE),
AFAIU dma_alloc_coherent() should mark the memory area uncachable .

We had this discussion a while back, when I submitted the fixes to make this driver work on the RK3399: dma_alloc_coherent only performs a memalign on ARM and ARM64:

See the following snippet in arch/arm/include/asm/dma-mapping.h:

static inline void *dma_alloc_coherent(size_t len, unsigned long *handle)

{
        *handle = (unsigned long)memalign(ARCH_DMA_MINALIGN, len);
        return (void *)*handle;
}

Quoted 10 lines

Quoted 5 lines

drivers/usb/dwc3/gadget.c +0 -0

 					  (unsigned long *)&dwc->ep0_trb_addr);
 	if (!dwc->ep0_trb) {
 		dev_err(dwc->dev, "failed to allocate ep0 trb\n");

Signature · 2 lines

Best regards,
Marek Vasut

Marek Vasut

2017-10-03 12:52 UTC #7 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 75 lines · Dr. Philipp Tomsich

Marek,
Quoted 67 lines · Marek Vasut
Quoted 1 line · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
You mean invalidation for inbound DMA, flush for outbound DMA, right ?
Quoted 32 lines
The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.
Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>
---
drivers/usb/dwc3/ep0.c | 7 ++++---
drivers/usb/dwc3/gadget.c | 3 ++-
2 files changed, 6 insertions(+), 4 deletions(-)
drivers/usb/dwc3/ep0.c +1 -1
index e61d980..f3a17a1 100644
@@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
 				| DWC3_TRB_CTRL_LST);
 
 	dwc3_flush_cache((uintptr_t)buf_dma, len);
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	if (chain)
 		return 0;
drivers/usb/dwc3/ep0.c static void dwc3_ep0_complete_data(struct dwc3 *dwc, · line 793
Quoted 5 lines
drivers/usb/dwc3/ep0.c +1 -1
@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 	if (!r)
 		return;
 
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Quoted 20 lines
drivers/usb/dwc3/ep0.c +2 -1
 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
 	if (status == DWC3_TRBSTS_SETUP_PENDING) {
@@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 			ur->actual += transferred;
 
 			trb++;
-			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
+					 sizeof(*trb) * 2);
 			length = trb->size & DWC3_TRB_SIZE_MASK;
 
 			ep0->free_slot = 0;
drivers/usb/dwc3/gadget.c +0 -0
index e065c5a..895a5bc 100644
@@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err0;
 	}
 
Quoted 5 lines
drivers/usb/dwc3/gadget.c int dwc3_gadget_init(struct dwc3 *dwc) · line 2571
Quoted 3 lines
drivers/usb/dwc3/gadget.c +2 -1
-	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
+	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
+						CACHELINE_SIZE),
AFAIU dma_alloc_coherent() should mark the memory area uncachable .

Quoted 5 lines

We had this discussion a while back, when I submitted the fixes to make this driver work on the RK3399: dma_alloc_coherent only performs a memalign on ARM and ARM64:
See the following snippet in arch/arm/include/asm/dma-mapping.h:

Does that mean the code is wrong / the function name is misleading ?

Quoted 5 lines

static inline void *dma_alloc_coherent(size_t len, unsigned long *handle)
{
        *handle = (unsigned long)memalign(ARCH_DMA_MINALIGN, len);
        return (void *)*handle;
}

[...]

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-03 13:17 UTC #8 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

In reply to Marek Vasut

Hi,

Quoted 4 lines · Marek Vasut

Quoted 1 line · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
You mean invalidation for inbound DMA, flush for outbound DMA, right ?

yes thats what i meant.

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +1 -1

-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);

Why *2 ?

Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.

Thanks,
Faiz

Marek Vasut

2017-10-03 13:18 UTC #9 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 12 lines · Faiz Abbas

Hi,
Quoted 4 lines · Marek Vasut
Quoted 1 line · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
You mean invalidation for inbound DMA, flush for outbound DMA, right ?
yes thats what i meant.
Quoted 1 line
Quoted 1 line

Quoted 8 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.

Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-04 10:51 UTC #10 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 18 lines · Marek Vasut

Quoted 17 lines · Faiz Abbas
Hi,
Quoted 4 lines · Marek Vasut
Quoted 1 line · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
You mean invalidation for inbound DMA, flush for outbound DMA, right ?
yes thats what i meant.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?

Quoted 7 lines

Quoted 3 lines
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?

A single trb is 16 bytes in size and two of them are allocated contiguously. Originally, a flush on the first trb was flushing both of them anyway as the minimum flush is CACHELINE_SIZE (64 bytes). This is not changing any functionality as far as I have tested. Just making sure cache misaligned warnings don't show up.

Thanks,
Faiz

Marek Vasut

2017-10-04 12:31 UTC #11 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 25 lines · Faiz Abbas

Hi,
Quoted 22 lines · Marek Vasut
Quoted 20 lines · Faiz Abbas
Hi,
Quoted 4 lines · Marek Vasut
Quoted 1 line · Faiz Abbas
A flush of the cache is required before any DMA access can take place.
You mean invalidation for inbound DMA, flush for outbound DMA, right ?
yes thats what i meant.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.

Quoted 5 lines

Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.

Why are two allocated continuously ? (I am not dwc3 expert)

Quoted 4 lines

Originally, a flush on the first trb was flushing both of them anyway as the minimum flush is CACHELINE_SIZE (64 bytes). This is not changing any functionality as far as I have tested. Just making sure cache misaligned warnings don't show up.

If you flush 64bytes, you flush more than 2 TRBs, you flush something around those TRBs too.

Quoted 3 lines

Thanks,
Faiz

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-04 13:11 UTC #12 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 17 lines · Marek Vasut

Quoted 16 lines · Faiz Abbas
Hi,
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Faiz Abbas
Hi,
Quoted 6 lines · Marek Vasut
Review
Quoted 3 lines · Faiz Abbas
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)

Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.

It'll be great if someone can shed light on this.

Quoted 8 lines

Quoted 5 lines
Originally, a flush on the first trb was flushing both of them anyway as the minimum flush is CACHELINE_SIZE (64 bytes). This is not changing any functionality as far as I have tested. Just making sure cache misaligned warnings don't show up.
If you flush 64bytes, you flush more than 2 TRBs, you flush something around those TRBs too.

Yes and that is why I changed the allocation step to ROUND(sizeof(trb) * 2, 64). We use only 32 bytes for two trbs but make sure to allocate 64 bytes so that we can safely flush it.

Thanks
Faiz

Marek Vasut

2017-10-05 11:27 UTC #13 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 26 lines · Faiz Abbas

Hi,
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Faiz Abbas
Hi,
Quoted 10 lines · Marek Vasut
Quoted 9 lines · Faiz Abbas
Hi,
Quoted 6 lines · Marek Vasut
Review
Quoted 3 lines · Faiz Abbas
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Quoted 7 lines
Quoted 3 lines
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.

Quoted 6 lines

Quoted 1 line
Why are two allocated continuously ? (I am not dwc3 expert)
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.

Maybe that's something you need to check -- why it fails if aligned . Do the TRBs need to be stored back-to-back ?

Quoted 18 lines

It'll be great if someone can shed light on this.
Quoted 8 lines
Quoted 5 lines
Originally, a flush on the first trb was flushing both of them anyway as the minimum flush is CACHELINE_SIZE (64 bytes). This is not changing any functionality as far as I have tested. Just making sure cache misaligned warnings don't show up.
If you flush 64bytes, you flush more than 2 TRBs, you flush something around those TRBs too.
Yes and that is why I changed the allocation step to ROUND(sizeof(trb) * 2, 64). We use only 32 bytes for two trbs but make sure to allocate 64 bytes so that we can safely flush it.
Thanks
Faiz

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-06 11:33 UTC #14 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 29 lines · Marek Vasut

Quoted 28 lines · Faiz Abbas
Hi,
Quoted 24 lines · Marek Vasut
Quoted 16 lines · Faiz Abbas
Hi,
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Faiz Abbas
Hi,
Quoted 6 lines · Marek Vasut
Review
Quoted 3 lines · Faiz Abbas
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)

Quoted 7 lines

Quoted 4 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
Maybe that's something you need to check -- why it fails if aligned . Do the TRBs need to be stored back-to-back ?

Sure. Will check that and submit another version with fixes.

Thanks,
Faiz

Faiz Abbas

author

2017-10-10 05:37 UTC #15 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

+Kishon

Quoted 45 lines · Faiz Abbas

Hi,
Quoted 29 lines · Marek Vasut
Quoted 28 lines · Faiz Abbas
Hi,
Quoted 24 lines · Marek Vasut
Quoted 16 lines · Faiz Abbas
Hi,
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Faiz Abbas
Hi,
Quoted 6 lines · Marek Vasut
Review
Quoted 3 lines · Faiz Abbas
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
Quoted 7 lines
Quoted 4 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
Maybe that's something you need to check -- why it fails if aligned . Do the TRBs need to be stored back-to-back ?
Sure. Will check that and submit another version with fixes.
Thanks,
Faiz

Kishon Vijay Abraham I

2017-10-10 05:48 UTC #16 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 34 lines · Faiz Abbas

+Kishon
Quoted 31 lines · Faiz Abbas
Hi,
Quoted 28 lines · Marek Vasut
Quoted 27 lines · Faiz Abbas
Hi,
Quoted 24 lines · Marek Vasut
Quoted 16 lines · Faiz Abbas
Hi,
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Faiz Abbas
Hi,
Quoted 6 lines · Marek Vasut
Review
Quoted 3 lines · Faiz Abbas
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)

The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address

|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.

It's not the code but it's the HW.

Thanks
Kishon

Marek Vasut

2017-10-10 08:00 UTC #17 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 1 line · Kishon Vijay Abraham I

Hi,

Hi,

[...]

Quoted 31 lines

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address
|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Quoted 6 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
It's not the code but it's the HW.

That'd imply we need either some sort of flushing scheme or non-cachable memory allocation. What does Linux do ?

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-10 10:45 UTC #18 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi Marek,

Quoted 43 lines · Marek Vasut

Quoted 1 line · Kishon Vijay Abraham I
Hi,
Hi,
[...]
Quoted 36 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address
|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.

Quoted 5 lines

Quoted 1 line
It's not the code but it's the HW.
That'd imply we need either some sort of flushing scheme or non-cachable memory allocation. What does Linux do ?
The dma_alloc_coherent in linux kernel allocates non-cachable memory.

Currently, the code is using local variable trb to flush the cache. When the first trb is used, dwc3_flush_cache flushes the complete CACHELINE_SIZE (including the 2nd trb).
When the 2nd trb is used to flush cache, since it is an unaligned flush, it will issue a warning and will align it to the lower cache line boundary (flushing the 1st trb in the process).

So with or without this patch, both trbs are getting flushed with every call. With the patch, we can just avoid misaligned messages by always flushing using an aligned address.

Thanks,
Faiz

Marek Vasut

2017-10-10 13:49 UTC #19 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 59 lines · Faiz Abbas

Hi Marek,
Quoted 43 lines · Marek Vasut
Quoted 1 line · Kishon Vijay Abraham I
Hi,
Hi,
[...]
Quoted 36 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
- dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address
|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
Quoted 5 lines
Quoted 1 line
It's not the code but it's the HW.
That'd imply we need either some sort of flushing scheme or non-cachable memory allocation. What does Linux do ?
The dma_alloc_coherent in linux kernel allocates non-cachable memory.
Currently, the code is using local variable trb to flush the cache. When the first trb is used, dwc3_flush_cache flushes the complete CACHELINE_SIZE (including the 2nd trb).
When the 2nd trb is used to flush cache, since it is an unaligned flush, it will issue a warning and will align it to the lower cache line boundary (flushing the 1st trb in the process).

Quoted 3 lines

So with or without this patch, both trbs are getting flushed with every call. With the patch, we can just avoid misaligned messages by always flushing using an aligned address.

What worries me is that you can flush something into the memory while the controller is writing into it as well. Is that possible ?

Signature · 2 lines

Best regards,
Marek Vasut

Faiz Abbas

author

2017-10-11 08:23 UTC #20 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 60 lines · Marek Vasut

Quoted 59 lines · Faiz Abbas
Hi Marek,
Quoted 43 lines · Marek Vasut
Quoted 1 line · Kishon Vijay Abraham I
Hi,
Hi,
[...]
Quoted 36 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
- dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not strictly required as dwc3_flush_cache() rounds up the size to CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address
|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
Quoted 5 lines
Quoted 1 line
It's not the code but it's the HW.
That'd imply we need either some sort of flushing scheme or non-cachable memory allocation. What does Linux do ?
The dma_alloc_coherent in linux kernel allocates non-cachable memory.
Currently, the code is using local variable trb to flush the cache. When the first trb is used, dwc3_flush_cache flushes the complete CACHELINE_SIZE (including the 2nd trb).
When the 2nd trb is used to flush cache, since it is an unaligned flush, it will issue a warning and will align it to the lower cache line boundary (flushing the 1st trb in the process).

Quoted 7 lines

Quoted 3 lines
So with or without this patch, both trbs are getting flushed with every call. With the patch, we can just avoid misaligned messages by always flushing using an aligned address.
What worries me is that you can flush something into the memory while the controller is writing into it as well. Is that possible ?

No, control to the hardware is only given after all the trbs have been configured and flushed to memory. This is done by using the chain variable in the code.

        dwc3_flush_cache((uintptr_t)buf_dma, len);
        dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);

        if (chain)
                return 0;

        memset(&params, 0, sizeof(params));
        params.param0 = upper_32_bits(dwc->ep0_trb_addr);
        params.param1 = lower_32_bits(dwc->ep0_trb_addr);

        ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
                        DWC3_DEPCMD_STARTTRANSFER, &params);

Thanks,
Faiz

Kishon Vijay Abraham I

2017-10-11 08:58 UTC #21 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Hi,

Quoted 70 lines · Faiz Abbas

Hi,
Quoted 60 lines · Marek Vasut
Quoted 59 lines · Faiz Abbas
Hi Marek,
Quoted 43 lines · Marek Vasut
Quoted 1 line · Kishon Vijay Abraham I
Hi,
Hi,
[...]
Quoted 36 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
- dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not
strictly required as dwc3_flush_cache() rounds up the size to
CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address
|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
Quoted 5 lines
Quoted 1 line
It's not the code but it's the HW.
That'd imply we need either some sort of flushing scheme or non-cachable memory allocation. What does Linux do ?
The dma_alloc_coherent in linux kernel allocates non-cachable memory.
Currently, the code is using local variable trb to flush the cache. When the first trb is used, dwc3_flush_cache flushes the complete CACHELINE_SIZE (including the 2nd trb).
When the 2nd trb is used to flush cache, since it is an unaligned flush, it will issue a warning and will align it to the lower cache line boundary (flushing the 1st trb in the process).
Quoted 7 lines
Quoted 3 lines
So with or without this patch, both trbs are getting flushed with every call. With the patch, we can just avoid misaligned messages by always flushing using an aligned address.
What worries me is that you can flush something into the memory while the controller is writing into it as well. Is that possible ?

Quoted 6 lines

No, control to the hardware is only given after all the trbs have been configured and flushed to memory. This is done by using the chain variable in the code.
        dwc3_flush_cache((uintptr_t)buf_dma, len);
        dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);

this flush_cache can be moved after the chain so that flush is only invoked after all the TRB's has been configured.

Thanks
Kishon

Faiz Abbas

author

2017-10-11 13:23 UTC #22 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

Quoted 77 lines · Kishon Vijay Abraham I

Hi,
Quoted 74 lines · Faiz Abbas
Hi,
Quoted 60 lines · Marek Vasut
Quoted 59 lines · Faiz Abbas
Hi Marek,
Quoted 43 lines · Marek Vasut
Quoted 1 line · Kishon Vijay Abraham I
Hi,
Hi,
[...]
Quoted 36 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
- dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
Why *2 ?
Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not
strictly required as dwc3_flush_cache() rounds up the size to
CACHELINE_SIZE but from a caller POV, flush everything we allocated.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size
would be better ?
A single trb is 16 bytes in size and two of them are allocated contiguously.
Why are two allocated continuously ? (I am not dwc3 expert)
The TRB's should be allocated contiguously for dwc3 and only the base of the entire TRB table is programmed in the HW.
________________ <------------------ TRB table base address
|     TRB0       |
|________________|
|     TRB1       |
|________________|
|     TRB2       |
|________________|
|     TRBn       |
|________________|
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Neither am I. I did try to pad to the dwc_trb structure such that each trb is 64 bytes in size but this leads to failures when testing. I didn't get a chance to debug this though. I suspect its because the code expects the trbs to be contiguous and/or 16 bytes in size.
Quoted 5 lines
Quoted 1 line
It's not the code but it's the HW.
That'd imply we need either some sort of flushing scheme or non-cachable memory allocation. What does Linux do ?
The dma_alloc_coherent in linux kernel allocates non-cachable memory.
Currently, the code is using local variable trb to flush the cache. When the first trb is used, dwc3_flush_cache flushes the complete CACHELINE_SIZE (including the 2nd trb).
When the 2nd trb is used to flush cache, since it is an unaligned flush, it will issue a warning and will align it to the lower cache line boundary (flushing the 1st trb in the process).
Quoted 7 lines
Quoted 3 lines
So with or without this patch, both trbs are getting flushed with every call. With the patch, we can just avoid misaligned messages by always flushing using an aligned address.
What worries me is that you can flush something into the memory while the controller is writing into it as well. Is that possible ?
No, control to the hardware is only given after all the trbs have been configured and flushed to memory. This is done by using the chain variable in the code.

Quoted 5 lines

Quoted 2 lines
        dwc3_flush_cache((uintptr_t)buf_dma, len);
        dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
this flush_cache can be moved after the chain so that flush is only invoked after all the TRB's has been configured.

Sure, that can be done.

Thanks,
Faiz

Marek Vasut

2017-10-03 13:01 UTC #23 lore raw

PATCH 1/1 dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

In reply to Faiz Abbas

Quoted 7 lines · Faiz Abbas

A flush of the cache is required before any DMA access can take place. The minimum size that can be flushed from the cache is one cache line size. Therefore, any buffer allocated for DMA should be in multiples of cache line size.
Thus, allocate memory for ep0_trb in multiples of cache line size.

Quoted 7 lines

Also, when local variable trb is assigned to dwc->ep0_trb[1] and used to flush cache, it leads to cache misaligned messages as only the base address dwc->ep0_trb is cache aligned.
Therefore, flush cache using ep0_trb_addr which is always cache aligned.
Signed-off-by Faiz Abbas <[email protected]>

btw the tags should be usb: dwc3:

Quoted 52 lines

---
 drivers/usb/dwc3/ep0.c    | 7 ++++---
 drivers/usb/dwc3/gadget.c | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

drivers/usb/dwc3/ep0.c +4 -3

index e61d980..f3a17a1 100644
@@ -82,7 +82,7 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
 				| DWC3_TRB_CTRL_LST);
 
 	dwc3_flush_cache((uintptr_t)buf_dma, len);
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	if (chain)
 		return 0;

@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 	if (!r)
 		return;
 
-	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
 
 	status = DWC3_TRB_SIZE_TRBSTS(trb->size);
 	if (status == DWC3_TRBSTS_SETUP_PENDING) {

@@ -821,7 +821,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 			ur->actual += transferred;
 
 			trb++;
-			dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
+			dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr,
+					 sizeof(*trb) * 2);
 			length = trb->size & DWC3_TRB_SIZE_MASK;
 
 			ep0->free_slot = 0;

drivers/usb/dwc3/gadget.c +2 -1

index e065c5a..895a5bc 100644
@@ -2567,7 +2567,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err0;
 	}
 
-	dwc->ep0_trb = dma_alloc_coherent(sizeof(*dwc->ep0_trb) * 2,
+	dwc->ep0_trb = dma_alloc_coherent(ROUND(sizeof(*dwc->ep0_trb) * 2,
+						CACHELINE_SIZE),
 					  (unsigned long *)&dwc->ep0_trb_addr);
 	if (!dwc->ep0_trb) {
 		dev_err(dwc->dev, "failed to allocate ep0 trb\n");

Signature · 2 lines

Best regards,
Marek Vasut